diff mbox series

[v3,2/3] drm/amdgpu: Remove redundant framebuffer format check

Message ID 20230113165919.580210-3-mcanal@igalia.com (mailing list archive)
State New, archived
Headers show
Series Check for valid framebuffer's format | expand

Commit Message

Maíra Canal Jan. 13, 2023, 4:59 p.m. UTC 
Now that framebuffer_check() verifies that the format is properly
supported, there is no need to check it again on amdgpu's inside
helpers.

Therefore, remove the redundant framebuffer format check from the
amdgpu_display_gem_fb_verify_and_init() function, letting
framebuffer_check() perform the framebuffer validation.

Reviewed-by: Simon Ser <contact@emersion.fr>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Simon Ser Jan. 13, 2023, 5:06 p.m. UTC | #1 
Hm, unfortunately I think we need to keep the check in amdgpu for the
same reason as i915: amdgpu will pick a modifier if user-space didn't
supply one on GFX9+.

I wonder if that also applies to vmwgfx? Maybe that would be a reason
to have the check in framebuffer_init()? (Not sure!)
Maíra Canal Jan. 13, 2023, 5:56 p.m. UTC | #2 
On 1/13/23 14:06, Simon Ser wrote:
> Hm, unfortunately I think we need to keep the check in amdgpu for the
> same reason as i915: amdgpu will pick a modifier if user-space didn't
> supply one on GFX9+.
> 
> I wonder if that also applies to vmwgfx? Maybe that would be a reason
> to have the check in framebuffer_init()? (Not sure!)

Considering that we could then remove the check from i915 and amdgpu if
we move the check to framebuffer_init(), I believe that this would be a
good reason to perform the check there. I'll send a v4 including the check
on framebuffer_init() and removing the check from i915.

Thanks for the feedback!

Best Regards,
- Maíra Canal
Maíra Canal Jan. 17, 2023, 2:37 p.m. UTC | #3 
Hi Simon,

On 1/13/23 14:06, Simon Ser wrote:
> Hm, unfortunately I think we need to keep the check in amdgpu for the
> same reason as i915: amdgpu will pick a modifier if user-space didn't
> supply one on GFX9+.
> 
> I wonder if that also applies to vmwgfx? Maybe that would be a reason
> to have the check in framebuffer_init()? (Not sure!)

I tried to move the check to framebuffer_init(), but it ended up causing
problems in the i915 driver (the kernel was emitting warnings when running
the IGT tests). I was thinking of going back to the drm_gem_fb_create()
approach [1], as it would make the other drivers return EINVAL in the case of
a bad modifier and it wouldn't change the current behavior of i915 and amdgpu.

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

Best Regards,
- Maíra Canal
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index b22471b3bd63..611b7a4b086c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1120,16 +1120,6 @@  static int amdgpu_display_gem_fb_verify_and_init(struct drm_device *dev,
 
 	rfb->base.obj[0] = obj;
 	drm_helper_mode_fill_fb_struct(dev, &rfb->base, mode_cmd);
-	/* Verify that the modifier is supported. */
-	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]);
-
-		ret = -EINVAL;
-		goto err;
-	}
 
 	ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
 	if (ret)