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

[Ville Syrjälä]

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.

> 
> 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? What would such a fallback mechanism
even be other than "let's assume linear"? I see no reason to inflict
that pain on every test that just wants to know "is this format
supported?".

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.

> 
> 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);
> >  

-- 
Ville Syrjälä
Intel