| Message ID | 20170124152716.16780-1-eric.engestrom@imgtec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks Eric, This does looks like a reasonable change to me. I don't think there are any legacy reasons for excluding VGEM from testing in DRIVER_ANY compatible tests. On 2017-01-24 10:27 AM, Eric Engestrom wrote: > Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com> > --- > Not tested or anything, I just happened to notice this code and it > looked wrong, but maybe I misunderstood what it was meant to do. > > An alternative would be to just set the bits the the drivers that are > defined already, but that would require an update everytime a new > DRIVER_* is added (error-prone): > #define DRIVER_ANY ((1 << 4) - 1) > --- > lib/drmtest.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/drmtest.h b/lib/drmtest.h > index 19d4bd19..1d41df93 100644 > --- a/lib/drmtest.h > +++ b/lib/drmtest.h > @@ -42,7 +42,7 @@ > #define DRIVER_VC4 (1 << 1) > #define DRIVER_VGEM (1 << 2) > #define DRIVER_VIRTIO (1 << 3) > -#define DRIVER_ANY ~(DRIVER_VGEM) > +#define DRIVER_ANY (~0) > > #ifdef ANDROID > #if (!(defined HAVE_MMAP64)) && (!(defined __x86_64__)) >
NAK. DRIVER_VGEM is omitted from DRIVER_ANY intentionally. Vgem is unable to modeset, unable to render, practically it only supports the vgem-specific tests. See also: lib/drmtest.c, __drm_open_driver(). -- Petri Latvala
Hi, On 30 January 2017 at 11:46, Petri Latvala <petri.latvala@intel.com> wrote: > NAK. > > DRIVER_VGEM is omitted from DRIVER_ANY intentionally. Vgem is unable > to modeset, unable to render, practically it only supports the > vgem-specific tests. See also: lib/drmtest.c, __drm_open_driver(). Yeah, I agree with this. It's mostly just there as an auxiliary helper. Opening DRIVER_ANY to vgem means that, if you run on a system with vgem as well as a supported driver, you can end up with a near-100% skip rate if you don't explicitly specify the device, depending on device-load ordering. Cheers, Daniel
On Monday, 2017-01-30 11:50:52 +0000, Daniel Stone wrote: > Hi, > > On 30 January 2017 at 11:46, Petri Latvala <petri.latvala@intel.com> wrote: > > NAK. > > > > DRIVER_VGEM is omitted from DRIVER_ANY intentionally. Vgem is unable > > to modeset, unable to render, practically it only supports the > > vgem-specific tests. See also: lib/drmtest.c, __drm_open_driver(). > > Yeah, I agree with this. It's mostly just there as an auxiliary > helper. Opening DRIVER_ANY to vgem means that, if you run on a system > with vgem as well as a supported driver, you can end up with a > near-100% skip rate if you don't explicitly specify the device, > depending on device-load ordering. > > Cheers, > Daniel OK. This should probably be documented then (a simple comment next to the #define would go a long way), so that the next guy who see this doesn't make the same mistake I did :) Cheers, Eric
On Mon, Jan 30, 2017 at 12:18:07PM +0000, Eric Engestrom wrote: > On Monday, 2017-01-30 11:50:52 +0000, Daniel Stone wrote: > > Hi, > > > > On 30 January 2017 at 11:46, Petri Latvala <petri.latvala@intel.com> wrote: > > > NAK. > > > > > > DRIVER_VGEM is omitted from DRIVER_ANY intentionally. Vgem is unable > > > to modeset, unable to render, practically it only supports the > > > vgem-specific tests. See also: lib/drmtest.c, __drm_open_driver(). > > > > Yeah, I agree with this. It's mostly just there as an auxiliary > > helper. Opening DRIVER_ANY to vgem means that, if you run on a system > > with vgem as well as a supported driver, you can end up with a > > near-100% skip rate if you don't explicitly specify the device, > > depending on device-load ordering. > > > > Cheers, > > Daniel > > OK. This should probably be documented then (a simple comment next to > the #define would go a long way), so that the next guy who see this > doesn't make the same mistake I did :) Maybe we should rename it to DRIVER_ANY_KMS, and check in the open function that there's at least 1 plane/crtc/encoder/connector there and otherwise skip? I think that would be even better than documenting it, since it'd also catch e.g. tegra+nouveau systems. -Daniel
diff --git a/lib/drmtest.h b/lib/drmtest.h index 19d4bd19..1d41df93 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -42,7 +42,7 @@ #define DRIVER_VC4 (1 << 1) #define DRIVER_VGEM (1 << 2) #define DRIVER_VIRTIO (1 << 3) -#define DRIVER_ANY ~(DRIVER_VGEM) +#define DRIVER_ANY (~0) #ifdef ANDROID #if (!(defined HAVE_MMAP64)) && (!(defined __x86_64__))
Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com> --- Not tested or anything, I just happened to notice this code and it looked wrong, but maybe I misunderstood what it was meant to do. An alternative would be to just set the bits the the drivers that are defined already, but that would require an update everytime a new DRIVER_* is added (error-prone): #define DRIVER_ANY ((1 << 4) - 1) --- lib/drmtest.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)