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

[Kazlauskas, Nicholas]

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

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