[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

✓ Fi.CI.BAT: success for series starting with [1/3] drm: Make sure at least one plane supports the fb format

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Make sure at least one plane supports the fb format
URL   : https://patchwork.freedesktop.org/series/39383/
State : success

== Summary ==

Series 39383v1 series starting with [1/3] drm: Make sure at least one plane supports the fb format
https://patchwork.freedesktop.org/api/1.0/series/39383/revisions/1/mbox/

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:421s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:371s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:480s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:453s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:394s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:559s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:492s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:571s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:411s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:289s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:507s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:406s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:447s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:108  pass:104  dwarn:0   dfail:0   fail:0   skip:3  
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:446s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:492s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:580s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:420s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:470s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:406s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:426s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:389s

4a3784737699f45784335e1c9446596bf7833606 drm-tip: 2018y-03m-05d-14h-32m-57s UTC integration manifest
90f13ce75e41 drm: Fix some coding style issues
91d199eeba2a drm/i915: Eliminate the horrendous format check code
2c14c910a7ca drm: Make sure at least one plane supports the fb format

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8232/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/3] drm: Make sure at least one plane supports the fb format

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Make sure at least one plane supports the fb format
URL   : https://patchwork.freedesktop.org/series/39383/
State : failure

== Summary ==

---- Possible new issues:

Test kms_vblank:
        Subgroup pipe-a-ts-continuation-modeset:
                skip       -> PASS       (shard-snb)
        Subgroup pipe-b-ts-continuation-suspend:
                pass       -> INCOMPLETE (shard-snb)

---- Known issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-right-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185 +1
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-atomic:
                skip       -> PASS       (shard-snb) fdo#102670
Test kms_flip:
        Subgroup 2x-flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887
        Subgroup dpms-vs-vblank-race:
                fail       -> PASS       (shard-hsw) fdo#103060
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047

fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3468 pass:1827 dwarn:1   dfail:0   fail:7   skip:1633 time:12186s
shard-hsw        total:3468 pass:1773 dwarn:1   dfail:0   fail:2   skip:1691 time:11686s
shard-snb        total:3387 pass:1335 dwarn:2   dfail:0   fail:1   skip:2048 time:6917s
Blacklisted hosts:
shard-kbl        total:3387 pass:1915 dwarn:1   dfail:0   fail:7   skip:1463 time:8970s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8232/shards.html
_______________________________________________
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

[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

✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Make sure at least one plane supports the fb format (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm: Make sure at least one plane supports the fb format (rev3)
URL   : https://patchwork.freedesktop.org/series/39383/
State : success

== Summary ==

Series 39383v3 series starting with [v2,1/3] drm: Make sure at least one plane supports the fb format
https://patchwork.freedesktop.org/api/1.0/series/39383/revisions/3/mbox/

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:422s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:369s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:507s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:487s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:479s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:468s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:408s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:290s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:516s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:394s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:409s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:465s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:417s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:468s
fi-kbl-7560u     total:108  pass:104  dwarn:0   dfail:0   fail:0   skip:3  
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:507s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:582s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:436s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:518s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:533s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:503s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:495s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:420s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:429s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:519s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:388s

82e049a0d7e6bcaae89a1e0774c8dc0d60b081a1 drm-tip: 2018y-03m-05d-18h-55m-22s UTC integration manifest
ea0839bcb7f2 drm: Fix some coding style issues
b2f4a47f3ed9 drm/i915: Eliminate the horrendous format check code
11545007e0a6 drm: Make sure at least one plane supports the fb format

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8235/issues.html
_______________________________________________
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

[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

✗ Fi.CI.IGT: failure for series starting with [v2,1/3] drm: Make sure at least one plane supports the fb format (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm: Make sure at least one plane supports the fb format (rev3)
URL   : https://patchwork.freedesktop.org/series/39383/
State : failure

== Summary ==

---- Possible new issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-a-256x256-bottom-edge:
                pass       -> DMESG-WARN (shard-hsw)
        Subgroup pipe-a-256x256-top-edge:
                pass       -> DMESG-WARN (shard-hsw)
        Subgroup pipe-a-64x64-right-edge:
                pass       -> INCOMPLETE (shard-hsw)
        Subgroup pipe-c-128x128-left-edge:
                dmesg-warn -> PASS       (shard-hsw)
        Subgroup pipe-c-256x256-right-edge:
                dmesg-warn -> PASS       (shard-hsw)
        Subgroup pipe-c-64x64-bottom-edge:
                dmesg-warn -> PASS       (shard-apl)

---- Known issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-bottom-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185 +2
Test kms_flip:
        Subgroup dpms-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
        Subgroup flip-vs-expired-vblank:
                fail       -> PASS       (shard-snb) fdo#102887
Test kms_frontbuffer_tracking:
        Subgroup fbc-rgb565-draw-mmap-cpu:
                fail       -> PASS       (shard-apl) fdo#101623
Test pm_lpsp:
        Subgroup screens-disabled:
                pass       -> FAIL       (shard-hsw) fdo#104941

fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941

shard-apl        total:3468 pass:1826 dwarn:1   dfail:0   fail:7   skip:1633 time:12377s
shard-hsw        total:3463 pass:1765 dwarn:3   dfail:0   fail:3   skip:1690 time:11611s
shard-snb        total:3468 pass:1363 dwarn:2   dfail:0   fail:2   skip:2101 time:7060s
Blacklisted hosts:
shard-kbl        total:3382 pass:1902 dwarn:3   dfail:0   fail:6   skip:1469 time:8784s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8235/shards.html
_______________________________________________
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

[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