[4/4] drm/vc4: Validate framebuffer pixel format/modifier

Commit Message

Ville Syrjälä March 9, 2018, 3:14 p.m. UTC

From: Ville Syrjälä <[e-mail]>

Only create framebuffers with supported format/modifier combinations by
checking that at least one plane supports the requested combination.

Using drm_any_plane_has_format() is somewhat suboptimal for vc4 since
the planes have (mostly) uniform capabilities. But I was lazy and
didn't feel like exporting drm_plane_format_check() and hand rolling
anything better. Also I really just wanted to come up with another
user for drm_any_plane_has_format() ;)

Compile tested only.

Cc: Eric Anholt <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Eric Anholt March 9, 2018, 8:54 p.m. UTC | #1

Ville Syrjala <[e-mail]> writes:

> From: Ville Syrjälä <[e-mail]>
>
> Only create framebuffers with supported format/modifier combinations by
> checking that at least one plane supports the requested combination.
>
> Using drm_any_plane_has_format() is somewhat suboptimal for vc4 since
> the planes have (mostly) uniform capabilities. But I was lazy and
> didn't feel like exporting drm_plane_format_check() and hand rolling
> anything better. Also I really just wanted to come up with another
> user for drm_any_plane_has_format() ;)

I'm not excited about vc4 having error-return behavior that other
drivers don't have.  Actually, I don't really see why we should be doing
this check in fb create at all, given that you have to do so again at
atomic_check time with the specific plane.

Could you just delete the i915 fb format check code?

Ville Syrjälä March 12, 2018, 3:04 p.m. UTC | #2

On Fri, Mar 09, 2018 at 12:54:43PM -0800, Eric Anholt wrote:
> Ville Syrjala <[e-mail]> writes:
> 
> > From: Ville Syrjälä <[e-mail]>
> >
> > Only create framebuffers with supported format/modifier combinations by
> > checking that at least one plane supports the requested combination.
> >
> > Using drm_any_plane_has_format() is somewhat suboptimal for vc4 since
> > the planes have (mostly) uniform capabilities. But I was lazy and
> > didn't feel like exporting drm_plane_format_check() and hand rolling
> > anything better. Also I really just wanted to come up with another
> > user for drm_any_plane_has_format() ;)
> 
> I'm not excited about vc4 having error-return behavior that other
> drivers don't have.  Actually, I don't really see why we should be doing
> this check in fb create at all, given that you have to do so again at
> atomic_check time with the specific plane.
> 
> Could you just delete the i915 fb format check code?

I don't want unsupported formats to get anywhere near the driver
code. Makes life much less stressful when you know what can and
can't get in.

Also I see no good reason to allow userspace to create fbs it
can't actually use later on. Much better to reject early and tell
userspace to pick another format. I imagine pre-blobifier userspace
could even use this as a way to probe what's supported by the driver.

Daniel Vetter March 12, 2018, 4:57 p.m. UTC | #3

On Fri, Mar 09, 2018 at 12:54:43PM -0800, Eric Anholt wrote:
> Ville Syrjala <[e-mail]> writes:
> 
> > From: Ville Syrj??l?? <[e-mail]>
> >
> > Only create framebuffers with supported format/modifier combinations by
> > checking that at least one plane supports the requested combination.
> >
> > Using drm_any_plane_has_format() is somewhat suboptimal for vc4 since
> > the planes have (mostly) uniform capabilities. But I was lazy and
> > didn't feel like exporting drm_plane_format_check() and hand rolling
> > anything better. Also I really just wanted to come up with another
> > user for drm_any_plane_has_format() ;)
> 
> I'm not excited about vc4 having error-return behavior that other
> drivers don't have.  Actually, I don't really see why we should be doing
> this check in fb create at all, given that you have to do so again at
> atomic_check time with the specific plane.
> 
> Could you just delete the i915 fb format check code?

Yeah I thought we agreed that any driver not using the compat primary
plane helper will get this checking by default? That seems simplest, and
safest too.
-Daniel

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ba60153dddb5..b6f15102dda0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -184,6 +184,17 @@  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])) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("unsupported pixel format %s / modifier 0x%llx\n",
+			      drm_get_format_name(mode_cmd->pixel_format,
+						  &format_name),
+			      mode_cmd->modifier[0]);
+		return ERR_PTR(-EINVAL);
+	}
+
 	return drm_gem_fb_create(dev, file_priv, mode_cmd);
 }