[PATCH] drm/vc4: Check for valid formats

[Maíra Canal]

On 1/11/23 10:28, Ville Syrjälä wrote:
> On Mon, Jan 02, 2023 at 03:20:06PM +0100, 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 at 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 at 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.
> 
> That was the approach I tried originally but there were a bunch of
> problems with various drivers it at the time. Dunno if all of those
> got sorted out or not. IIRC the idea floating around for ancient
> drivers was to skip the check based on plane->format_default. Looks
> like we're already using that approach in the setcrtc ioctl.
> 

I ended up following Thomas's idea to check drm_drv_uses_atomic_modesetting()
in order to check the modifier only for atomic drivers. If you have any feedback
on this idea, I would be glad to hear it. The current version of this patch is [1].

[1] https://lore.kernel.org/dri-devel/20230109105807.18172-1-mcanal@igalia.com/T/

Best Regards,
- Maíra Canal