dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/4] drm: Add drm_any_plane_has_format()
@ 2018-03-09 15:14 Ville Syrjala
  2018-03-09 15:14 ` [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code Ville Syrjala
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Ville Syrjala @ 2018-03-09 15:14 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a function to check whether there is at least one plane that
supports a specific format and modifier combination. Drivers can
use this to reject unsupported formats/modifiers in .fb_create().

v2: Accept anyformat if the driver doesn't do planes (Eric)
    s/planes_have_format/any_plane_has_format/ (Eric)
    Check the modifier as well since we already have a function
    that does both
v3: Don't do the check in the core since we may not know the
    modifier yet, instead export the function and let drivers
    call it themselves

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c   | 23 +++++++++++++++++++++++
 include/drm/drm_mode_config.h |  6 ++++++
 include/drm/drm_plane.h       |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index a5d1fc7e8a37..3b2d6f8d889d 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -578,6 +578,29 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	return 0;
 }
 
+/**
+ * drm_any_plane_has_format - Check whether any plane supports this format and modifier combination
+ * @dev: DRM device
+ * @format: pixel format (DRM_FORMAT_*)
+ * @modifier: data layout modifier
+ *
+ * Returns:
+ * Whether at least one plane supports the specified format and modifier combination.
+ */
+bool drm_any_plane_has_format(struct drm_device *dev,
+			      u32 format, u64 modifier)
+{
+	struct drm_plane *plane;
+
+	drm_for_each_plane(plane, dev) {
+		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_any_plane_has_format);
+
 /*
  * __setplane_internal - setplane handler for internal callers
  *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7569f22ffef6..9b894de9a75d 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -52,6 +52,12 @@ struct drm_mode_config_funcs {
 	 * requested metadata, but most of that is left to the driver. See
 	 * &struct drm_mode_fb_cmd2 for details.
 	 *
+	 * To validate the pixel format and modifier drivers can use
+	 * drm_any_plane_has_format() to make sure at least one plane supports
+	 * the requested values. Note that the driver must first determine the
+	 * actual modifier used if the request doesn't have it specified,
+	 * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
+	 *
 	 * If the parameters are deemed valid and the backing storage objects in
 	 * the underlying memory manager all exist, then the driver allocates
 	 * a new &drm_framebuffer structure, subclassed to contain
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index f7bf4a48b1c3..930e8fdd90f8 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -683,5 +683,7 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 #define drm_for_each_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
 
+bool drm_any_plane_has_format(struct drm_device *dev,
+			      u32 format, u64 modifier);
 
 #endif
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code
  2018-03-09 15:14 [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Ville Syrjala
@ 2018-03-09 15:14 ` Ville Syrjala
  2018-10-26 20:08   ` [Intel-gfx] " Dhinakaran Pandiyan
  2018-03-09 15:14 ` [PATCH 3/4] drm: Fix some coding style issues Ville Syrjala
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjala @ 2018-03-09 15:14 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the messy framebuffer format/modifier validation code
with a single call to drm_any_plane_has_format(). The code was
extremely annoying to maintain as you had to have a lot of platform
checks for different formats. The new code requires zero maintenance.

v2: Nuke the modifier checks as well since the core does that too now
v3: Call drm_any_plane_has_format() from the driver code

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 90 ++++--------------------------------
 1 file changed, 8 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2933ad38094f..7f06fa83d894 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13989,7 +13989,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
-	struct drm_format_name_buf format_name;
 	u32 pitch_limit;
 	unsigned int tiling, stride;
 	int ret = -EINVAL;
@@ -14020,33 +14019,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		}
 	}
 
-	/* Passed in modifier sanity checking. */
-	switch (mode_cmd->modifier[0]) {
-	case I915_FORMAT_MOD_Y_TILED_CCS:
-	case I915_FORMAT_MOD_Yf_TILED_CCS:
-		switch (mode_cmd->pixel_format) {
-		case DRM_FORMAT_XBGR8888:
-		case DRM_FORMAT_ABGR8888:
-		case DRM_FORMAT_XRGB8888:
-		case DRM_FORMAT_ARGB8888:
-			break;
-		default:
-			DRM_DEBUG_KMS("RC supported only with RGB8888 formats\n");
-			goto err;
-		}
-		/* fall through */
-	case I915_FORMAT_MOD_Y_TILED:
-	case I915_FORMAT_MOD_Yf_TILED:
-		if (INTEL_GEN(dev_priv) < 9) {
-			DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n",
-				      mode_cmd->modifier[0]);
-			goto err;
-		}
-	case DRM_FORMAT_MOD_LINEAR:
-	case I915_FORMAT_MOD_X_TILED:
-		break;
-	default:
-		DRM_DEBUG_KMS("Unsupported fb modifier 0x%llx!\n",
+	if (!drm_any_plane_has_format(&dev_priv->drm,
+				      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]);
 		goto err;
 	}
@@ -14081,60 +14061,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
-	/* Reject formats not supported by any plane early. */
-	switch (mode_cmd->pixel_format) {
-	case DRM_FORMAT_C8:
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_ARGB8888:
-		break;
-	case DRM_FORMAT_XRGB1555:
-		if (INTEL_GEN(dev_priv) > 3) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_ABGR8888:
-		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
-		    INTEL_GEN(dev_priv) < 9) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-		if (INTEL_GEN(dev_priv) < 4) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_ABGR2101010:
-		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_VYUY:
-		if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	default:
-		DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-			      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-		goto err;
-	}
-
 	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
 	if (mode_cmd->offsets[0] != 0)
 		goto err;
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] drm: Fix some coding style issues
  2018-03-09 15:14 [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Ville Syrjala
  2018-03-09 15:14 ` [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code Ville Syrjala
@ 2018-03-09 15:14 ` Ville Syrjala
  2018-03-09 15:14 ` [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier Ville Syrjala
  2018-10-26 20:04 ` [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Dhinakaran Pandiyan
  3 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjala @ 2018-03-09 15:14 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Put an empty line between the variable declarations and the code, and
use tabs for alignment.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index c0530a1af5e3..7df025669067 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -162,9 +162,10 @@ static int framebuffer_check(struct drm_device *dev,
 	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
 	if (!info) {
 		struct drm_format_name_buf format_name;
+
 		DRM_DEBUG_KMS("bad framebuffer format %s\n",
-		              drm_get_format_name(r->pixel_format,
-		                                  &format_name));
+			      drm_get_format_name(r->pixel_format,
+						  &format_name));
 		return -EINVAL;
 	}
 
-- 
2.16.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier
  2018-03-09 15:14 [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Ville Syrjala
  2018-03-09 15:14 ` [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code Ville Syrjala
  2018-03-09 15:14 ` [PATCH 3/4] drm: Fix some coding style issues Ville Syrjala
@ 2018-03-09 15:14 ` Ville Syrjala
  2018-03-09 20:54   ` Eric Anholt
  2018-10-26 20:04 ` [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Dhinakaran Pandiyan
  3 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjala @ 2018-03-09 15:14 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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 <eric@anholt.net>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

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);
 }
 
-- 
2.16.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier
  2018-03-09 15:14 ` [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier Ville Syrjala
@ 2018-03-09 20:54   ` Eric Anholt
  2018-03-12 15:04     ` Ville Syrjälä
  2018-03-12 16:57     ` Daniel Vetter
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Anholt @ 2018-03-09 20:54 UTC (permalink / raw)
  To: Ville Syrjala, dri-devel; +Cc: intel-gfx


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

Ville Syrjala <ville.syrjala@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier
  2018-03-09 20:54   ` Eric Anholt
@ 2018-03-12 15:04     ` Ville Syrjälä
  2018-03-12 16:57     ` Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2018-03-12 15:04 UTC (permalink / raw)
  To: Eric Anholt; +Cc: intel-gfx, dri-devel

On Fri, Mar 09, 2018 at 12:54:43PM -0800, Eric Anholt wrote:
> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier
  2018-03-09 20:54   ` Eric Anholt
  2018-03-12 15:04     ` Ville Syrjälä
@ 2018-03-12 16:57     ` Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2018-03-12 16:57 UTC (permalink / raw)
  To: Eric Anholt; +Cc: intel-gfx, dri-devel

On Fri, Mar 09, 2018 at 12:54:43PM -0800, Eric Anholt wrote:
> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> 
> > From: Ville Syrj??l?? <ville.syrjala@linux.intel.com>
> >
> > 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/4] drm: Add drm_any_plane_has_format()
  2018-03-09 15:14 [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Ville Syrjala
                   ` (2 preceding siblings ...)
  2018-03-09 15:14 ` [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier Ville Syrjala
@ 2018-10-26 20:04 ` Dhinakaran Pandiyan
  3 siblings, 0 replies; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-26 20:04 UTC (permalink / raw)
  To: Ville Syrjala, dri-devel; +Cc: intel-gfx

On Fri, 2018-03-09 at 17:14 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a function to check whether there is at least one plane that
> supports a specific format and modifier combination. Drivers can
> use this to reject unsupported formats/modifiers in .fb_create().
> 
> v2: Accept anyformat if the driver doesn't do planes (Eric)
>     s/planes_have_format/any_plane_has_format/ (Eric)
>     Check the modifier as well since we already have a function
>     that does both
> v3: Don't do the check in the core since we may not know the
>     modifier yet, instead export the function and let drivers
>     call it themselves
> 
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I ended up writing a similar patch for i915. Having this in the core
seems better and patch still applies cleanly.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> ---
>  drivers/gpu/drm/drm_plane.c   | 23 +++++++++++++++++++++++
>  include/drm/drm_mode_config.h |  6 ++++++
>  include/drm/drm_plane.h       |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> index a5d1fc7e8a37..3b2d6f8d889d 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -578,6 +578,29 @@ int drm_plane_check_pixel_format(struct
> drm_plane *plane,
>  	return 0;
>  }
>  
> +/**
> + * drm_any_plane_has_format - Check whether any plane supports this
> format and modifier combination
> + * @dev: DRM device
> + * @format: pixel format (DRM_FORMAT_*)
> + * @modifier: data layout modifier
> + *
> + * Returns:
> + * Whether at least one plane supports the specified format and
> modifier combination.
> + */
> +bool drm_any_plane_has_format(struct drm_device *dev,
> +			      u32 format, u64 modifier)
> +{
> +	struct drm_plane *plane;
> +
> +	drm_for_each_plane(plane, dev) {
> +		if (drm_plane_check_pixel_format(plane, format,
> modifier) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_any_plane_has_format);
> +
>  /*
>   * __setplane_internal - setplane handler for internal callers
>   *
> diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> index 7569f22ffef6..9b894de9a75d 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -52,6 +52,12 @@ struct drm_mode_config_funcs {
>  	 * requested metadata, but most of that is left to the driver.
> See
>  	 * &struct drm_mode_fb_cmd2 for details.
>  	 *
> +	 * To validate the pixel format and modifier drivers can use
> +	 * drm_any_plane_has_format() to make sure at least one plane
> supports
> +	 * the requested values. Note that the driver must first
> determine the
> +	 * actual modifier used if the request doesn't have it
> specified,
> +	 * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> +	 *
>  	 * If the parameters are deemed valid and the backing storage
> objects in
>  	 * the underlying memory manager all exist, then the driver
> allocates
>  	 * a new &drm_framebuffer structure, subclassed to contain
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a48b1c3..930e8fdd90f8 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -683,5 +683,7 @@ static inline struct drm_plane
> *drm_plane_find(struct drm_device *dev,
>  #define drm_for_each_plane(plane, dev) \
>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list,
> head)
>  
> +bool drm_any_plane_has_format(struct drm_device *dev,
> +			      u32 format, u64 modifier);
>  
>  #endif

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Intel-gfx] [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code
  2018-03-09 15:14 ` [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code Ville Syrjala
@ 2018-10-26 20:08   ` Dhinakaran Pandiyan
  0 siblings, 0 replies; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-26 20:08 UTC (permalink / raw)
  To: Ville Syrjala, dri-devel; +Cc: intel-gfx

On Fri, 2018-03-09 at 17:14 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the messy framebuffer format/modifier validation code
> with a single call to drm_any_plane_has_format(). The code was
> extremely annoying to maintain as you had to have a lot of platform
> checks for different formats. The new code requires zero maintenance.
> 
> v2: Nuke the modifier checks as well since the core does that too now
> v3: Call drm_any_plane_has_format() from the driver code
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Patch looks good to me, but does not apply cleanly now.


> ---
>  drivers/gpu/drm/i915/intel_display.c | 90 ++++--------------------
> ------------
>  1 file changed, 8 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2933ad38094f..7f06fa83d894 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13989,7 +13989,6 @@ static int intel_framebuffer_init(struct
> intel_framebuffer *intel_fb,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>  	struct drm_framebuffer *fb = &intel_fb->base;
> -	struct drm_format_name_buf format_name;
>  	u32 pitch_limit;
>  	unsigned int tiling, stride;
>  	int ret = -EINVAL;
> @@ -14020,33 +14019,14 @@ static int intel_framebuffer_init(struct
> intel_framebuffer *intel_fb,
>  		}
>  	}
>  
> -	/* Passed in modifier sanity checking. */
> -	switch (mode_cmd->modifier[0]) {
> -	case I915_FORMAT_MOD_Y_TILED_CCS:
> -	case I915_FORMAT_MOD_Yf_TILED_CCS:
> -		switch (mode_cmd->pixel_format) {
> -		case DRM_FORMAT_XBGR8888:
> -		case DRM_FORMAT_ABGR8888:
> -		case DRM_FORMAT_XRGB8888:
> -		case DRM_FORMAT_ARGB8888:
> -			break;
> -		default:
> -			DRM_DEBUG_KMS("RC supported only with RGB8888
> formats\n");
> -			goto err;
> -		}
> -		/* fall through */
> -	case I915_FORMAT_MOD_Y_TILED:
> -	case I915_FORMAT_MOD_Yf_TILED:
> -		if (INTEL_GEN(dev_priv) < 9) {
> -			DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n",
> -				      mode_cmd->modifier[0]);
> -			goto err;
> -		}
> -	case DRM_FORMAT_MOD_LINEAR:
> -	case I915_FORMAT_MOD_X_TILED:
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("Unsupported fb modifier 0x%llx!\n",
> +	if (!drm_any_plane_has_format(&dev_priv->drm,
> +				      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]);
>  		goto err;
>  	}
> @@ -14081,60 +14061,6 @@ static int intel_framebuffer_init(struct
> intel_framebuffer *intel_fb,
>  		goto err;
>  	}
>  
> -	/* Reject formats not supported by any plane early. */
> -	switch (mode_cmd->pixel_format) {
> -	case DRM_FORMAT_C8:
> -	case DRM_FORMAT_RGB565:
> -	case DRM_FORMAT_XRGB8888:
> -	case DRM_FORMAT_ARGB8888:
> -		break;
> -	case DRM_FORMAT_XRGB1555:
> -		if (INTEL_GEN(dev_priv) > 3) {
> -			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -				      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -			goto err;
> -		}
> -		break;
> -	case DRM_FORMAT_ABGR8888:
> -		if (!IS_VALLEYVIEW(dev_priv) &&
> !IS_CHERRYVIEW(dev_priv) &&
> -		    INTEL_GEN(dev_priv) < 9) {
> -			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -				      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -			goto err;
> -		}
> -		break;
> -	case DRM_FORMAT_XBGR8888:
> -	case DRM_FORMAT_XRGB2101010:
> -	case DRM_FORMAT_XBGR2101010:
> -		if (INTEL_GEN(dev_priv) < 4) {
> -			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -				      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -			goto err;
> -		}
> -		break;
> -	case DRM_FORMAT_ABGR2101010:
> -		if (!IS_VALLEYVIEW(dev_priv) &&
> !IS_CHERRYVIEW(dev_priv)) {
> -			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -				      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -			goto err;
> -		}
> -		break;
> -	case DRM_FORMAT_YUYV:
> -	case DRM_FORMAT_UYVY:
> -	case DRM_FORMAT_YVYU:
> -	case DRM_FORMAT_VYUY:
> -		if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) {
> -			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -				      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -			goto err;
> -		}
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> -			      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> -		goto err;
> -	}
> -
>  	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
>  	if (mode_cmd->offsets[0] != 0)
>  		goto err;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-26 20:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-09 15:14 [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Ville Syrjala
2018-03-09 15:14 ` [PATCH v3 2/4] drm/i915: Eliminate the horrendous format check code Ville Syrjala
2018-10-26 20:08   ` [Intel-gfx] " Dhinakaran Pandiyan
2018-03-09 15:14 ` [PATCH 3/4] drm: Fix some coding style issues Ville Syrjala
2018-03-09 15:14 ` [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier Ville Syrjala
2018-03-09 20:54   ` Eric Anholt
2018-03-12 15:04     ` Ville Syrjälä
2018-03-12 16:57     ` Daniel Vetter
2018-10-26 20:04 ` [PATCH v3 1/4] drm: Add drm_any_plane_has_format() Dhinakaran Pandiyan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).