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@xxxxxxxxxx> --- 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.cindex 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 planeis 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 Idecided to introduce the check in the driver instead of the API.[1] https://cgit.freedesktop.org/drm/drm/tree/Documentation/gpu/todo.rst#n279Best Regards, - Maíra CanalBest 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:
OpenPGP_signature
Description: OpenPGP digital signature
