[PATCH] drm/vc4: Check for valid formats

[PATCH] drm/vc4: Check for valid formats

[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);
+	}
+
+	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

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

[Thomas Zimmermann]

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

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.

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

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

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

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

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

[Thomas Zimmermann]

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

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

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

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

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

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

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

[Thomas Zimmermann]

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

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

[Daniel Vetter]

On Mon, Jan 02, 2023 at 04:21:55PM +0100, 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.

^^  this sounds like a good idea. Also note that for these checks the
right check is actually drm_drv_uses_atomic_modesetting, since it's about
the driver interface. DRIVER_ATOMIC is more about whether the driver
interface actually upholds the atomic/tearfree guarantees userspace
expects.
-Daniel

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




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

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

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.

-- 
Ville Syrjälä
Intel

Re: [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@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.
> 
> 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

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

[Daniel Vetter]

On Wed, Jan 11, 2023 at 11:32:43AM -0300, Maíra Canal wrote:
> 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@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.
> > 
> > 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/

Yeah for atomic drivers the format list better be accurate, so we should
be able to enforce this.

For legacy driver it's a bit a mess, but I'm toying with some ideas how we
could add at least some validation there too. It's just that the audit is
a total pain :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch