Re: [PATCH] drm/vc4: Check for valid formats

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5648 bytes --]

Hi

Am 02.01.23 um 16:39 schrieb Maíra Canal:
> On 1/2/23 12:21, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 02.01.23 um 15:29 schrieb Maíra Canal:
>>> Hi,
>>>
>>> On 1/2/23 11:20, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 02.01.23 um 14:57 schrieb Maíra Canal:
>>>>> Currently, vc4 is not checking valid formats before creating a
>>>>> framebuffer, which is triggering the IGT test
>>>>> igt@kms_addfb_basic@addfb25-bad-modifier to fail, as vc4 accepts
>>>>> to create a framebuffer with an invalid modifier. Therefore, check
>>>>> for valid formats before creating framebuffers on vc4 and vc5 in
>>>>> order to avoid creating framebuffers with invalid formats.
>>>>>
>>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>>> ---
>>>>>   drivers/gpu/drm/vc4/vc4_kms.c | 23 ++++++++++++++++++++++-
>>>>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c 
>>>>> b/drivers/gpu/drm/vc4/vc4_kms.c
>>>>> index 53d9f30460cf..5d1afd66fcc1 100644
>>>>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>>>>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>>>>> @@ -500,6 +500,27 @@ static struct drm_framebuffer 
>>>>> *vc4_fb_create(struct drm_device *dev,
>>>>>         mode_cmd = &mode_cmd_local;
>>>>>     }
>>>>>
>>>>> +    if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
>>>>> +                      mode_cmd->modifier[0])) {
>>>>> +        drm_dbg_kms(dev, "Unsupported pixel format %p4cc / 
>>>>> modifier 0x%llx\n",
>>>>> +                &mode_cmd->pixel_format, mode_cmd->modifier[0]);
>>>>> +        return ERR_PTR(-EINVAL);
>>>>> +    }
>>>>
>>>> This might be a stupid question, but why doesn't 
>>>> drm_fbgem_fb_create() do this test already? It seems like a 
>>>> no-brainer and *not* testing for the plane formats should be the 
>>>> exception.
>>>
>>> I thought the same initially, but then I found this mention on the 
>>> TODO list [1]. So, it
>>> is not possible to test it on drm_gem_fb_create() because, for 
>>> non-atomic, checking
>>> drm_any_plane_has_format() is not possible since like the format list 
>>> for the primary plane
>>> is fake and we'd therefor reject valid formats.
>>
>> Thanks for the pointer to the docs.
>>
>> I see two options:
>>
>> 1) Testing for atomic modesetting can be done via 
>> drm_core_check_feature(dev, DRIVER_ATOMIC).  If true, 
>> drm_gem_fb_create() can further test for the plane formats. For 
>> non-atomic drivers, just skip the format test.
>>
>> 2) As an alternative, we could invert the IGT test and explicitly 
>> allow any format to be allocated. Almost no drivers currently bother 
>> with the format test anyway. And DRM will already fail if userspace 
>> attaches a framebuffer to a plane with incompatible formats. [1]  (I'd 
>> personally prefer this option, but I'm not sure if there's any 
>> consensus on that.)
> 
> I guess the second option will probably break Intel's CI, which I'm not 
> sure if it is a good
> idea. Maybe the first option is a bit less intrusive and will help the 
> DRM helper to have the
> same behavior as drivers like i915 and AMDGPU.

That makes some sense. I'd like to hear the opinion of the affected 
developers. If no one shows up, go for option 1 then. It has the 
potential to break someone's userspace code, but it's easily revert-able.

Best regards
Thomas

> 
> Thanks for the suggestions!
> 
> Best Regards,
> - Maíra Canal
> 
>>
>> With either implemented, the TODO item could be remvoed AFAICT.
>>
>> Best regards
>> Thomas
>>
>> [1] 
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_plane.c#L789
>>
>>>
>>> I'm not sure if anything changed since this was written, but that was 
>>> the reason that I
>>> decided to introduce the check in the driver instead of the API.
>>>
>>> [1] 
>>> https://cgit.freedesktop.org/drm/drm/tree/Documentation/gpu/todo.rst#n279
>>>
>>> Best Regards,
>>> - Maíra Canal
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> +
>>>>> +    return drm_gem_fb_create(dev, file_priv, mode_cmd);
>>>>> +}
>>>>> +
>>>>> +static struct drm_framebuffer *vc5_fb_create(struct drm_device *dev,
>>>>> +                         struct drm_file *file_priv,
>>>>> +                         const struct drm_mode_fb_cmd2 *mode_cmd)
>>>>> +{
>>>>> +    if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
>>>>> +                      mode_cmd->modifier[0])) {
>>>>> +        drm_dbg_kms(dev, "Unsupported pixel format %p4cc / 
>>>>> modifier 0x%llx\n",
>>>>> +                &mode_cmd->pixel_format, mode_cmd->modifier[0]);
>>>>> +        return ERR_PTR(-EINVAL);
>>>>> +    }
>>>>> +
>>>>>     return drm_gem_fb_create(dev, file_priv, mode_cmd);
>>>>>   }
>>>>>
>>>>> @@ -1033,7 +1054,7 @@ static const struct drm_mode_config_funcs 
>>>>> vc4_mode_funcs = {
>>>>>   static const struct drm_mode_config_funcs vc5_mode_funcs = {
>>>>>     .atomic_check = vc4_atomic_check,
>>>>>     .atomic_commit = drm_atomic_helper_commit,
>>>>> -    .fb_create = drm_gem_fb_create,
>>>>> +    .fb_create = vc5_fb_create,
>>>>>   };
>>>>>
>>>>>   int vc4_kms_load(struct drm_device *dev)
>>>>> -- 
>>>>> 2.38.1
>>>>>
>>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]