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);
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;
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;
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; }
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));-- 2.16.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
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? :)
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
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
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
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
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
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.
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;
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
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
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Eric Anholt -
Harry Wentland -
Ville Syrjala -
Ville Syrjälä