[igt-dev] [PATCH i-g-t 01/25] lib/igt_kms: Fill the plane format/mod information for pre-blobifier drivers

[Paulo Zanoni]

Em Sex, 2018-09-21 às 16:26 +0300, Ville Syrjälä escreveu:
> On Thu, Sep 20, 2018 at 01:26:42PM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-07-19 às 18:03 +0300, Ville Syrjala escreveu:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > For drivers that don't support the IN_FORMATS blob we should just
> > > consult the format list returned by getplane. Since we can't know
> > > which modifiers are supported we'll assume linear-only. Obviously
> > > that may not work for every driver out there, but not much more
> > > we can do unless we start to actually probing with addfb.
> > 
> > Can you please elaborate on why we need that change? What exactly
> > do we
> > gain from it?
> 
> We gain the ability to write tests without worrying about IN_FORMATS
> everywhere.
> 
> > IMHO there's always the worry that this could be misused
> > in the future, restricting tests to linear only when other formats
> > are
> > indeed possible. Having NULL buffers crashing the test is a good
> > way to
> > prevent misuse.
> 
> I don't know what kind of misuse you're thinking of. Actually not
> sure what misuse in this context would mean really.


Thinking that everything is linear only when in fact other formats are
actually supported.


> 
> > 
> > What if we populate plane->formats like we do here but leave plane-
> > > modifiers NULL? It would allow other pieces of the code to rely
> > > on the
> > 
> > format list while not exposing the risk of the "linear only"
> > incorrect
> > restriction.
> 
> So every test would have to check for !modifiers and come up with
> some kind of fallback mechanism?

Yes, if IN_FORMATS is not present you have to fallback to the old "try
it until you don't get an EINVAL". Isn't this the old protocol?


>  What would such a fallback mechanism
> even be other than "let's assume linear"?

The same mechanism we used before we created IN_FORMATS.


>  I see no reason to inflict
> that pain on every test that just wants to know "is this format
> supported?".

The way I see, not populating a wrong table helps the test more than
setting a table with data that may not reflect reality.

> 
> Also any driver that advertizes formats that don't support linear
> and doesn't expose IN_FORMATS clearly doesn't care about supporting
> driver/hw agnostic userspace, so having some tests fail for them
> seems perfectly reasonable to me.

So older i915 didn't care about supporting driver/hw agnostic user
space?

> 
> > 
> > That said, the patch is correct in the sense that it does what it
> > proposes to do.
> > 
> > > 
> > > Cc: Ulrich Hecht <ulrich.hecht+renesas at gmail.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  lib/igt_kms.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index 476a786233c0..5641d8c1cf7c 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -4068,8 +4068,28 @@ static void
> > > igt_fill_plane_format_mod(igt_display_t *display, igt_plane_t
> > > *plane
> > >  	int idx = 0;
> > >  	int count;
> > >  
> > > -	if (!igt_plane_has_prop(plane, IGT_PLANE_IN_FORMATS))
> > > +	if (!igt_plane_has_prop(plane, IGT_PLANE_IN_FORMATS)) {
> > > +		drmModePlanePtr p = plane->drm_plane;
> > > +
> > > +		count = p->count_formats;
> > > +
> > > +		plane->format_mod_count = count;
> > > +		plane->formats = calloc(count, sizeof(plane-
> > > > formats[0]));
> > > 
> > > +		igt_assert(plane->formats);
> > > +		plane->modifiers = calloc(count, sizeof(plane-
> > > > modifiers[0]));
> > > 
> > > +		igt_assert(plane->modifiers);
> > > +
> > > +		/*
> > > +		 * We don't know which modifiers are
> > > +		 * supported, so we'll assume linear only.
> > > +		 */
> > > +		for (int i = 0; i < count; i++) {
> > > +			plane->formats[i] = p->formats[i];
> > > +			plane->modifiers[i] =
> > > DRM_FORMAT_MOD_LINEAR;
> > > +		}
> > > +
> > >  		return;
> > > +	}
> > >  
> > >  	blob_id = igt_plane_get_prop(plane,
> > > IGT_PLANE_IN_FORMATS);
> > >  
> 
>