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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 27, 2018 at 08:30:39PM +0000, Kazlauskas, Nicholas wrote:
> On 9/21/18 5:39 PM, Paulo Zanoni wrote:
> > 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@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@gmail.com>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This patch allows for the igt_plane_has_format_mod and 
> igt_display_has_format_mod helpers to be used on drivers that don't 
> support IN_FORMATS (amdgpu being an example).
> 
> The helpers are really useful for fixing up IGT tests that run and fail 
> on hardware that isn't i915 but use i915 tiling modifiers. This allows 
> helps remove a lot of the genid checks to see if y/yf tiling is supported.
> 
> Without this patch the supported modifier list for these helpers is 
> empty so everything ends up skipping.
> 
> I've tested this patch with amdgpu along with the helpers and it works well.
> 
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Thanks. Pushed.

> 
> >>>> ---
> >>>>   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);
> >>>>   
> >>
> >>
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> 

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev