[PATCH 1/3] drm: Make sure at least one plane supports the fb format

[PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjala]

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

To make life easier for drivers, let's have the core check that the
requested pixel format is supported by at least one plane when creating
a new framebuffer.

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

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index c0530a1af5e3..155b21e579c4 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,6 +152,23 @@ static int fb_plane_height(int height,
 	return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool planes_have_format(struct drm_device *dev, u32 format)
+{
+	struct drm_plane *plane;
+
+	/* TODO: maybe maintain a device level format list? */
+	drm_for_each_plane(plane, dev) {
+		int i;
+
+		for (i = 0; i < plane->format_count; i++) {
+			if (plane->format_types[i] == format)
+				return true;
+		}
+	}
+
+	return false;
+}
+
 static int framebuffer_check(struct drm_device *dev,
 			     const struct drm_mode_fb_cmd2 *r)
 {
@@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	if (!planes_have_format(dev, r->pixel_format)) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
+			      drm_get_format_name(r->pixel_format,
+						  &format_name));
+		return -EINVAL;
+	}
+
 	/* now let the driver pick its own format info */
 	info = drm_get_format_info(dev, r);
 
-- 
2.16.1

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

[PATCH 2/3] drm/i915: Eliminate the horrendous format check code

[Ville Syrjala]

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

Now that the core makes sure that the pixel format is supported
by at least one plane, we can eliminate the hand rolled code for the
same thing in i915. The way we were doing was extremely inconvenient
because not only did you have to add the format to the plane's format
list, but you also had to come up with the correct platform checks
for this code.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb08590b4d40..9b03d50d2d53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13991,7 +13991,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;
@@ -14083,60 +14082,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

[PATCH 3/3] drm: Fix some coding style issues

[Ville Syrjala]

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 155b21e579c4..6aa86a6549a0 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -179,9 +179,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

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

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

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

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..155b21e579c4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>  	return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool planes_have_format(struct drm_device *dev, u32 format)
> +{
> +	struct drm_plane *plane;
> +
> +	/* TODO: maybe maintain a device level format list? */
> +	drm_for_each_plane(plane, dev) {
> +		int i;
> +
> +		for (i = 0; i < plane->format_count; i++) {
> +			if (plane->format_types[i] == format)
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>  			     const struct drm_mode_fb_cmd2 *r)
>  {
> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (!planes_have_format(dev, r->pixel_format)) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> +			      drm_get_format_name(r->pixel_format,
> +						  &format_name));
> +		return -EINVAL;
> +	}
> +

Won't this break KMS on things like the radeon driver, which doesn't do
planes?  Maybe check if any universal planes have been registered and
only do the check in that case?

Also, "any_planes_have_format()" might be slightly more descriptive.

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

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

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

Re: [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > To make life easier for drivers, let's have the core check that the
> > requested pixel format is supported by at least one plane when creating
> > a new framebuffer.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index c0530a1af5e3..155b21e579c4 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
> >  	return DIV_ROUND_UP(height, format->vsub);
> >  }
> >  
> > +static bool planes_have_format(struct drm_device *dev, u32 format)
> > +{
> > +	struct drm_plane *plane;
> > +
> > +	/* TODO: maybe maintain a device level format list? */
> > +	drm_for_each_plane(plane, dev) {
> > +		int i;
> > +
> > +		for (i = 0; i < plane->format_count; i++) {
> > +			if (plane->format_types[i] == format)
> > +				return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int framebuffer_check(struct drm_device *dev,
> >  			     const struct drm_mode_fb_cmd2 *r)
> >  {
> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (!planes_have_format(dev, r->pixel_format)) {
> > +		struct drm_format_name_buf format_name;
> > +
> > +		DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> > +			      drm_get_format_name(r->pixel_format,
> > +						  &format_name));
> > +		return -EINVAL;
> > +	}
> > +
> 
> Won't this break KMS on things like the radeon driver, which doesn't do
> planes?  Maybe check if any universal planes have been registered and
> only do the check in that case?

Hmm. I thought we add the implicit planes always. Apparently
drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
this would break all other formats, which is probably a bit too
aggressive.

I guess I could just skip the check in case any plane has
plane->format_default set. That should be indicating that the driver
doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
Harry?

> 
> Also, "any_planes_have_format()" might be slightly more descriptive.

Or any_plane_has_format()? Is that more englishy? :)

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

Re: [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Alex Deucher]

On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>>
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > To make life easier for drivers, let's have the core check that the
>> > requested pixel format is supported by at least one plane when creating
>> > a new framebuffer.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > index c0530a1af5e3..155b21e579c4 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> >     return DIV_ROUND_UP(height, format->vsub);
>> >  }
>> >
>> > +static bool planes_have_format(struct drm_device *dev, u32 format)
>> > +{
>> > +   struct drm_plane *plane;
>> > +
>> > +   /* TODO: maybe maintain a device level format list? */
>> > +   drm_for_each_plane(plane, dev) {
>> > +           int i;
>> > +
>> > +           for (i = 0; i < plane->format_count; i++) {
>> > +                   if (plane->format_types[i] == format)
>> > +                           return true;
>> > +           }
>> > +   }
>> > +
>> > +   return false;
>> > +}
>> > +
>> >  static int framebuffer_check(struct drm_device *dev,
>> >                          const struct drm_mode_fb_cmd2 *r)
>> >  {
>> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> >             return -EINVAL;
>> >     }
>> >
>> > +   if (!planes_have_format(dev, r->pixel_format)) {
>> > +           struct drm_format_name_buf format_name;
>> > +
>> > +           DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> > +                         drm_get_format_name(r->pixel_format,
>> > +                                             &format_name));
>> > +           return -EINVAL;
>> > +   }
>> > +
>>
>> Won't this break KMS on things like the radeon driver, which doesn't do
>> planes?  Maybe check if any universal planes have been registered and
>> only do the check in that case?
>
> Hmm. I thought we add the implicit planes always. Apparently
> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
> this would break all other formats, which is probably a bit too
> aggressive.
>
> I guess I could just skip the check in case any plane has
> plane->format_default set. That should be indicating that the driver
> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> Harry?

Probably leftover from DC bringup?

Alex

>
>>
>> Also, "any_planes_have_format()" might be slightly more descriptive.
>
> Or any_plane_has_format()? Is that more englishy? :)
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjala]

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

To make life easier for drivers, let's have the core check that the
requested pixel format is supported by at least one plane when creating
a new framebuffer.

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

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 43 +++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index c0530a1af5e3..b9a33e3f13e9 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,12 +152,37 @@ static int fb_plane_height(int height,
 	return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool any_plane_has_format(struct drm_device *dev,
+				 u32 format, u64 modifier)
+{
+	struct drm_plane *plane;
+
+	/* TODO: maybe maintain a device level format list? */
+	drm_for_each_plane(plane, dev) {
+		/*
+		 * In case the driver doesn't really do
+		 * planes we have to accept any format here.
+		 */
+		if (plane->format_default)
+			return true;
+
+		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
+			return true;
+	}
+
+	return false;
+}
+
 static int framebuffer_check(struct drm_device *dev,
 			     const struct drm_mode_fb_cmd2 *r)
 {
 	const struct drm_format_info *info;
+	u64 modifier = 0;
 	int i;
 
+	if (r->flags & DRM_MODE_FB_MODIFIERS)
+		modifier = r->modifier[0];
+
 	/* check if the format is supported at all */
 	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
 	if (!info) {
@@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
+			      drm_get_format_name(r->pixel_format, &format_name),
+			      modifier);
+		return -EINVAL;
+	}
+
 	/* now let the driver pick its own format info */
 	info = drm_get_format_info(dev, r);
 
@@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
 			return -EINVAL;
 		}
 
-		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
-			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
-				      r->modifier[i], i);
-			return -EINVAL;
-		}
-
-		if (r->flags & DRM_MODE_FB_MODIFIERS &&
-		    r->modifier[i] != r->modifier[0]) {
+		if (r->modifier[i] != modifier) {
 			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
 				      r->modifier[i], i);
 			return -EINVAL;
-- 
2.16.1

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

[PATCH v2 2/3] drm/i915: Eliminate the horrendous format check code

[Ville Syrjala]

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

Now that the core makes sure that the pixel format is supported
by at least one plane, we can eliminate the hand rolled code for the
same thing in i915. The way we were doing was extremely inconvenient
because not only did you have to add the format to the plane's format
list, but you also had to come up with the correct platform checks
for this code.

v2: Nuke the modifier checks as well since the core does that too now

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb08590b4d40..3ed1ef05cb55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13991,7 +13991,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;
@@ -14022,37 +14021,6 @@ 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",
-			      mode_cmd->modifier[0]);
-		goto err;
-	}
-
 	/*
 	 * gen2/3 display engine uses the fence if present,
 	 * so the tiling mode must match the fb modifier exactly.
@@ -14083,60 +14051,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

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

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Harry Wentland]

On 2018-03-05 04:33 PM, Alex Deucher wrote:
> On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>>>
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> To make life easier for drivers, let's have the core check that the
>>>> requested pixel format is supported by at least one plane when creating
>>>> a new framebuffer.
>>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>>> index c0530a1af5e3..155b21e579c4 100644
>>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>>> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>>>>     return DIV_ROUND_UP(height, format->vsub);
>>>>  }
>>>>
>>>> +static bool planes_have_format(struct drm_device *dev, u32 format)
>>>> +{
>>>> +   struct drm_plane *plane;
>>>> +
>>>> +   /* TODO: maybe maintain a device level format list? */
>>>> +   drm_for_each_plane(plane, dev) {
>>>> +           int i;
>>>> +
>>>> +           for (i = 0; i < plane->format_count; i++) {
>>>> +                   if (plane->format_types[i] == format)
>>>> +                           return true;
>>>> +           }
>>>> +   }
>>>> +
>>>> +   return false;
>>>> +}
>>>> +
>>>>  static int framebuffer_check(struct drm_device *dev,
>>>>                          const struct drm_mode_fb_cmd2 *r)
>>>>  {
>>>> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>>>>             return -EINVAL;
>>>>     }
>>>>
>>>> +   if (!planes_have_format(dev, r->pixel_format)) {
>>>> +           struct drm_format_name_buf format_name;
>>>> +
>>>> +           DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>>>> +                         drm_get_format_name(r->pixel_format,
>>>> +                                             &format_name));
>>>> +           return -EINVAL;
>>>> +   }
>>>> +
>>>
>>> Won't this break KMS on things like the radeon driver, which doesn't do
>>> planes?  Maybe check if any universal planes have been registered and
>>> only do the check in that case?
>>
>> Hmm. I thought we add the implicit planes always. Apparently
>> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
>> this would break all other formats, which is probably a bit too
>> aggressive.
>>
>> I guess I could just skip the check in case any plane has
>> plane->format_default set. That should be indicating that the driver
>> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
>> Harry?
> 
> Probably leftover from DC bringup?

I'm not sure if I'm following. Which flag are we talking about?

Is the concern the DC driver or amdgpu with DC (or radeon)?

DC does not call drm_crtc_init(). It initializes universal planes in amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().

From a quick glance this patch looks fine by me.

Harry

> 
> Alex
> 
>>
>>>
>>> Also, "any_planes_have_format()" might be slightly more descriptive.
>>
>> Or any_plane_has_format()? Is that more englishy? :)
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

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

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

> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>> 
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > To make life easier for drivers, let's have the core check that the
>> > requested pixel format is supported by at least one plane when creating
>> > a new framebuffer.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > index c0530a1af5e3..155b21e579c4 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> >  	return DIV_ROUND_UP(height, format->vsub);
>> >  }
>> >  
>> > +static bool planes_have_format(struct drm_device *dev, u32 format)
>> > +{
>> > +	struct drm_plane *plane;
>> > +
>> > +	/* TODO: maybe maintain a device level format list? */
>> > +	drm_for_each_plane(plane, dev) {
>> > +		int i;
>> > +
>> > +		for (i = 0; i < plane->format_count; i++) {
>> > +			if (plane->format_types[i] == format)
>> > +				return true;
>> > +		}
>> > +	}
>> > +
>> > +	return false;
>> > +}
>> > +
>> >  static int framebuffer_check(struct drm_device *dev,
>> >  			     const struct drm_mode_fb_cmd2 *r)
>> >  {
>> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> >  		return -EINVAL;
>> >  	}
>> >  
>> > +	if (!planes_have_format(dev, r->pixel_format)) {
>> > +		struct drm_format_name_buf format_name;
>> > +
>> > +		DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> > +			      drm_get_format_name(r->pixel_format,
>> > +						  &format_name));
>> > +		return -EINVAL;
>> > +	}
>> > +
>> 
>> Won't this break KMS on things like the radeon driver, which doesn't do
>> planes?  Maybe check if any universal planes have been registered and
>> only do the check in that case?
>
> Hmm. I thought we add the implicit planes always. Apparently
> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
> this would break all other formats, which is probably a bit too
> aggressive.
>
> I guess I could just skip the check in case any plane has
> plane->format_default set. That should be indicating that the driver
> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> Harry?
>
>> 
>> Also, "any_planes_have_format()" might be slightly more descriptive.
>
> Or any_plane_has_format()? Is that more englishy? :)

I like it.

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

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

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

Re: [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Mon, Mar 05, 2018 at 05:44:16PM -0500, Harry Wentland wrote:
> On 2018-03-05 04:33 PM, Alex Deucher wrote:
> > On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> >>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> >>>
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> To make life easier for drivers, let's have the core check that the
> >>>> requested pixel format is supported by at least one plane when creating
> >>>> a new framebuffer.
> >>>>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
> >>>>  1 file changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >>>> index c0530a1af5e3..155b21e579c4 100644
> >>>> --- a/drivers/gpu/drm/drm_framebuffer.c
> >>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >>>> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
> >>>>     return DIV_ROUND_UP(height, format->vsub);
> >>>>  }
> >>>>
> >>>> +static bool planes_have_format(struct drm_device *dev, u32 format)
> >>>> +{
> >>>> +   struct drm_plane *plane;
> >>>> +
> >>>> +   /* TODO: maybe maintain a device level format list? */
> >>>> +   drm_for_each_plane(plane, dev) {
> >>>> +           int i;
> >>>> +
> >>>> +           for (i = 0; i < plane->format_count; i++) {
> >>>> +                   if (plane->format_types[i] == format)
> >>>> +                           return true;
> >>>> +           }
> >>>> +   }
> >>>> +
> >>>> +   return false;
> >>>> +}
> >>>> +
> >>>>  static int framebuffer_check(struct drm_device *dev,
> >>>>                          const struct drm_mode_fb_cmd2 *r)
> >>>>  {
> >>>> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
> >>>>             return -EINVAL;
> >>>>     }
> >>>>
> >>>> +   if (!planes_have_format(dev, r->pixel_format)) {
> >>>> +           struct drm_format_name_buf format_name;
> >>>> +
> >>>> +           DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> >>>> +                         drm_get_format_name(r->pixel_format,
> >>>> +                                             &format_name));
> >>>> +           return -EINVAL;
> >>>> +   }
> >>>> +
> >>>
> >>> Won't this break KMS on things like the radeon driver, which doesn't do
> >>> planes?  Maybe check if any universal planes have been registered and
> >>> only do the check in that case?
> >>
> >> Hmm. I thought we add the implicit planes always. Apparently
> >> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
> >> this would break all other formats, which is probably a bit too
> >> aggressive.
> >>
> >> I guess I could just skip the check in case any plane has
> >> plane->format_default set. That should be indicating that the driver
> >> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> >> Harry?
> > 
> > Probably leftover from DC bringup?
> 
> I'm not sure if I'm following. Which flag are we talking about?

In amdgpu_dm_plane_init().

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-      switch (aplane->base.type) {
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-      case DRM_PLANE_TYPE_PRIMARY:
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:              aplane->base.format_default = true;
                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-              res = drm_universal_plane_init(

> 
> Is the concern the DC driver or amdgpu with DC (or radeon)?
> 
> DC does not call drm_crtc_init(). It initializes universal planes in amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
> 
> >From a quick glance this patch looks fine by me.
> 
> Harry
> 
> > 
> > Alex
> > 
> >>
> >>>
> >>> Also, "any_planes_have_format()" might be slightly more descriptive.
> >>
> >> Or any_plane_has_format()? Is that more englishy? :)
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 3/3] drm: Fix some coding style issues

[Daniel Vetter]

On Mon, Mar 05, 2018 at 04:49:19PM +0200, Ville Syrjala wrote:
> 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>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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 155b21e579c4..6aa86a6549a0 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -179,9 +179,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

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

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Daniel Vetter]

On Mon, Mar 05, 2018 at 05:44:16PM -0500, Harry Wentland wrote:
> On 2018-03-05 04:33 PM, Alex Deucher wrote:
> > On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> >>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> >>>
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> To make life easier for drivers, let's have the core check that the
> >>>> requested pixel format is supported by at least one plane when creating
> >>>> a new framebuffer.
> >>>>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
> >>>>  1 file changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >>>> index c0530a1af5e3..155b21e579c4 100644
> >>>> --- a/drivers/gpu/drm/drm_framebuffer.c
> >>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >>>> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
> >>>>     return DIV_ROUND_UP(height, format->vsub);
> >>>>  }
> >>>>
> >>>> +static bool planes_have_format(struct drm_device *dev, u32 format)
> >>>> +{
> >>>> +   struct drm_plane *plane;
> >>>> +
> >>>> +   /* TODO: maybe maintain a device level format list? */
> >>>> +   drm_for_each_plane(plane, dev) {
> >>>> +           int i;
> >>>> +
> >>>> +           for (i = 0; i < plane->format_count; i++) {
> >>>> +                   if (plane->format_types[i] == format)
> >>>> +                           return true;
> >>>> +           }
> >>>> +   }
> >>>> +
> >>>> +   return false;
> >>>> +}
> >>>> +
> >>>>  static int framebuffer_check(struct drm_device *dev,
> >>>>                          const struct drm_mode_fb_cmd2 *r)
> >>>>  {
> >>>> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
> >>>>             return -EINVAL;
> >>>>     }
> >>>>
> >>>> +   if (!planes_have_format(dev, r->pixel_format)) {
> >>>> +           struct drm_format_name_buf format_name;
> >>>> +
> >>>> +           DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> >>>> +                         drm_get_format_name(r->pixel_format,
> >>>> +                                             &format_name));
> >>>> +           return -EINVAL;
> >>>> +   }
> >>>> +
> >>>
> >>> Won't this break KMS on things like the radeon driver, which doesn't do
> >>> planes?  Maybe check if any universal planes have been registered and
> >>> only do the check in that case?
> >>
> >> Hmm. I thought we add the implicit planes always. Apparently
> >> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
> >> this would break all other formats, which is probably a bit too
> >> aggressive.
> >>
> >> I guess I could just skip the check in case any plane has
> >> plane->format_default set. That should be indicating that the driver
> >> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> >> Harry?
> > 
> > Probably leftover from DC bringup?
> 
> I'm not sure if I'm following. Which flag are we talking about?
> 
> Is the concern the DC driver or amdgpu with DC (or radeon)?
> 
> DC does not call drm_crtc_init(). It initializes universal planes in amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
> 
> From a quick glance this patch looks fine by me.

From amdgpu_dm_plane_init:

		aplane->base.format_default = true;
^^ this is entirely bogus, pls remove.

		res = drm_universal_plane_init(
				dm->adev->ddev,
				&aplane->base,
				possible_crtcs,
				&dm_plane_funcs,
				rgb_formats,
				ARRAY_SIZE(rgb_formats),
				NULL, aplane->base.type, NULL);


Wrt discussions: Also checking whether any plane has set
plane->format_default and aborting the check should keep old kms drivers
working I hope.

Cheers, Daniel

> 
> Harry
> 
> > 
> > Alex
> > 
> >>
> >>>
> >>> Also, "any_planes_have_format()" might be slightly more descriptive.
> >>
> >> Or any_plane_has_format()? Is that more englishy? :)
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Mon, Mar 05, 2018 at 11:41:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
> 
> 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
> 
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 43 +++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..b9a33e3f13e9 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,12 +152,37 @@ static int fb_plane_height(int height,
>  	return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool any_plane_has_format(struct drm_device *dev,
> +				 u32 format, u64 modifier)
> +{
> +	struct drm_plane *plane;
> +
> +	/* TODO: maybe maintain a device level format list? */
> +	drm_for_each_plane(plane, dev) {
> +		/*
> +		 * In case the driver doesn't really do
> +		 * planes we have to accept any format here.
> +		 */
> +		if (plane->format_default)
> +			return true;
> +
> +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>  			     const struct drm_mode_fb_cmd2 *r)
>  {
>  	const struct drm_format_info *info;
> +	u64 modifier = 0;
>  	int i;
>  
> +	if (r->flags & DRM_MODE_FB_MODIFIERS)
> +		modifier = r->modifier[0];

And of course this isn't quite correct. For the !modifiers case we're
now checking if any plane supports the format with the linear modifier,
but the actual modifier will only be picked by the driver later.

Not quite sure how I would write this in a way that really makes
sense. Might have to make it the driver's responsibility to call
any_plane_has_format() instead...

> +
>  	/* check if the format is supported at all */
>  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>  	if (!info) {
> @@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
> +			      drm_get_format_name(r->pixel_format, &format_name),
> +			      modifier);
> +		return -EINVAL;
> +	}
> +
>  	/* now let the driver pick its own format info */
>  	info = drm_get_format_info(dev, r);
>  
> @@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> -			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> -				      r->modifier[i], i);
> -			return -EINVAL;
> -		}
> -
> -		if (r->flags & DRM_MODE_FB_MODIFIERS &&
> -		    r->modifier[i] != r->modifier[0]) {
> +		if (r->modifier[i] != modifier) {
>  			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
>  				      r->modifier[i], i);
>  			return -EINVAL;
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 01:57:26PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 05, 2018 at 11:41:02PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To make life easier for drivers, let's have the core check that the
> > requested pixel format is supported by at least one plane when creating
> > a new framebuffer.
> > 
> > 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
> > 
> > Cc: Eric Anholt <eric@anholt.net>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 43 +++++++++++++++++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index c0530a1af5e3..b9a33e3f13e9 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -152,12 +152,37 @@ static int fb_plane_height(int height,
> >  	return DIV_ROUND_UP(height, format->vsub);
> >  }
> >  
> > +static bool any_plane_has_format(struct drm_device *dev,
> > +				 u32 format, u64 modifier)
> > +{
> > +	struct drm_plane *plane;
> > +
> > +	/* TODO: maybe maintain a device level format list? */
> > +	drm_for_each_plane(plane, dev) {
> > +		/*
> > +		 * In case the driver doesn't really do
> > +		 * planes we have to accept any format here.
> > +		 */
> > +		if (plane->format_default)
> > +			return true;
> > +
> > +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int framebuffer_check(struct drm_device *dev,
> >  			     const struct drm_mode_fb_cmd2 *r)
> >  {
> >  	const struct drm_format_info *info;
> > +	u64 modifier = 0;
> >  	int i;
> >  
> > +	if (r->flags & DRM_MODE_FB_MODIFIERS)
> > +		modifier = r->modifier[0];
> 
> And of course this isn't quite correct. For the !modifiers case we're
> now checking if any plane supports the format with the linear modifier,
> but the actual modifier will only be picked by the driver later.
> 
> Not quite sure how I would write this in a way that really makes
> sense. Might have to make it the driver's responsibility to call
> any_plane_has_format() instead...

Hmm. One idea would be to stick the check into drm_framebuffer_init(),
but I'm not comfortable with that since the driver may well be doing
things in its .fb_create() hook that expects the format+modifier to
have been validated already, and drm_framebuffer_init() should only be
called once everything is ready.

I guess I'll just export drm_any_plane_has_format() and add some
kerneldoc for .fb_create() to let people know about it. Better ideas
are welcome of course.

> 
> > +
> >  	/* check if the format is supported at all */
> >  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
> >  	if (!info) {
> > @@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
> > +		struct drm_format_name_buf format_name;
> > +
> > +		DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
> > +			      drm_get_format_name(r->pixel_format, &format_name),
> > +			      modifier);
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* now let the driver pick its own format info */
> >  	info = drm_get_format_info(dev, r);
> >  
> > @@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
> >  			return -EINVAL;
> >  		}
> >  
> > -		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> > -			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > -				      r->modifier[i], i);
> > -			return -EINVAL;
> > -		}
> > -
> > -		if (r->flags & DRM_MODE_FB_MODIFIERS &&
> > -		    r->modifier[i] != r->modifier[0]) {
> > +		if (r->modifier[i] != modifier) {
> >  			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> >  				      r->modifier[i], i);
> >  			return -EINVAL;
> > -- 
> > 2.16.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Harry Wentland]

On 2018-03-06 05:35 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 05:44:16PM -0500, Harry Wentland wrote:
>> On 2018-03-05 04:33 PM, Alex Deucher wrote:
>>> On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>>> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>>>>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>>>>>
>>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>
>>>>>> To make life easier for drivers, let's have the core check that the
>>>>>> requested pixel format is supported by at least one plane when creating
>>>>>> a new framebuffer.
>>>>>>
>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_framebuffer.c | 26 ++++++++++++++++++++++++++
>>>>>>  1 file changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>>>>> index c0530a1af5e3..155b21e579c4 100644
>>>>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>>>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>>>>> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>>>>>>     return DIV_ROUND_UP(height, format->vsub);
>>>>>>  }
>>>>>>
>>>>>> +static bool planes_have_format(struct drm_device *dev, u32 format)
>>>>>> +{
>>>>>> +   struct drm_plane *plane;
>>>>>> +
>>>>>> +   /* TODO: maybe maintain a device level format list? */
>>>>>> +   drm_for_each_plane(plane, dev) {
>>>>>> +           int i;
>>>>>> +
>>>>>> +           for (i = 0; i < plane->format_count; i++) {
>>>>>> +                   if (plane->format_types[i] == format)
>>>>>> +                           return true;
>>>>>> +           }
>>>>>> +   }
>>>>>> +
>>>>>> +   return false;
>>>>>> +}
>>>>>> +
>>>>>>  static int framebuffer_check(struct drm_device *dev,
>>>>>>                          const struct drm_mode_fb_cmd2 *r)
>>>>>>  {
>>>>>> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>>>>>>             return -EINVAL;
>>>>>>     }
>>>>>>
>>>>>> +   if (!planes_have_format(dev, r->pixel_format)) {
>>>>>> +           struct drm_format_name_buf format_name;
>>>>>> +
>>>>>> +           DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>>>>>> +                         drm_get_format_name(r->pixel_format,
>>>>>> +                                             &format_name));
>>>>>> +           return -EINVAL;
>>>>>> +   }
>>>>>> +
>>>>>
>>>>> Won't this break KMS on things like the radeon driver, which doesn't do
>>>>> planes?  Maybe check if any universal planes have been registered and
>>>>> only do the check in that case?
>>>>
>>>> Hmm. I thought we add the implicit planes always. Apparently
>>>> drm_crtc_init() adds a primary with X/ARGB8888, but no more. So
>>>> this would break all other formats, which is probably a bit too
>>>> aggressive.
>>>>
>>>> I guess I could just skip the check in case any plane has
>>>> plane->format_default set. That should be indicating that the driver
>>>> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
>>>> Harry?
>>>
>>> Probably leftover from DC bringup?
>>
>> I'm not sure if I'm following. Which flag are we talking about?
>>
>> Is the concern the DC driver or amdgpu with DC (or radeon)?
>>
>> DC does not call drm_crtc_init(). It initializes universal planes in amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
>>
>> From a quick glance this patch looks fine by me.
> 
> From amdgpu_dm_plane_init:
> 
> 		aplane->base.format_default = true;
> ^^ this is entirely bogus, pls remove.
> 

Makes sense. At one point we had a comment "for legacy code only" but that got lost. I'll clean it up.

Harry

> 		res = drm_universal_plane_init(
> 				dm->adev->ddev,
> 				&aplane->base,
> 				possible_crtcs,
> 				&dm_plane_funcs,
> 				rgb_formats,
> 				ARRAY_SIZE(rgb_formats),
> 				NULL, aplane->base.type, NULL);
> 
> 
> Wrt discussions: Also checking whether any plane has set
> plane->format_default and aborting the check should keep old kms drivers
> working I hope.
> 
> Cheers, Daniel
> 
>>
>> Harry
>>
>>>
>>> Alex
>>>
>>>>
>>>>>
>>>>> Also, "any_planes_have_format()" might be slightly more descriptive.
>>>>
>>>> Or any_plane_has_format()? Is that more englishy? :)
>>>>
>>>> --
>>>> Ville Syrjälä
>>>> Intel OTC
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel