| Message ID | 20181026013256.30808-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/plane: Export drm_plane_check_pixel_format() | expand |
On Thu, Oct 25, 2018 at 06:32:56PM -0700, Dhinakaran Pandiyan wrote: > Currently there is some duplication of pixel format and modifier > validation code between the fb creation and plane check paths. We can > unify them by checking if any plane supports a pixel format and modifier > combination during framebuffer creation. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 116 ++++++----------------------------- > 1 file changed, 19 insertions(+), 97 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe045abb6472..1b5d936a93d0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14362,6 +14362,19 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, > DRM_MODE_ROTATE_0); > } > > +static bool any_plane_supports_format(struct drm_i915_private *dev_priv, > + uint64_t fb_modifier, > + uint32_t pixel_format) "format first, modifier second" is the typical covention. But I think we could stuff this entire thing into the core, in case someone else wants to reuse it. I think I even posted the patches that do it like that. Ah yes: https://patchwork.freedesktop.org/series/39700/ > +{ > + struct drm_plane *plane; > + > + drm_for_each_plane(plane, &dev_priv->drm) > + if (!drm_plane_check_pixel_format(plane, pixel_format, > + fb_modifier)) > + return true; > + return false; > +} > + > static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > struct drm_i915_gem_object *obj, > struct drm_mode_fb_cmd2 *mode_cmd) > @@ -14399,40 +14412,12 @@ 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_Yf_TILED: > - if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > - DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n"); > - goto err; > - } > - /* fall through */ > - case I915_FORMAT_MOD_Y_TILED: > - if (INTEL_GEN(dev_priv) < 9) { > - DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n", > - mode_cmd->modifier[0]); > - goto err; > - } > - break; > - 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]); > + if (!any_plane_supports_format(dev_priv, mode_cmd->modifier[0], > + mode_cmd->pixel_format)) { > + DRM_DEBUG_KMS("Unsupported pixel format %s or modifier 0x%llx\n", > + drm_get_format_name(mode_cmd->pixel_format, > + &format_name), > + mode_cmd->modifier[0]); > goto err; > } > > @@ -14466,69 +14451,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; > - case DRM_FORMAT_NV12: > - if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) || > - IS_BROXTON(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.14.1
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, October 26, 2018 7:46 AM > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 2/2] drm/i915: Reuse plane format modifier checks to > verify addfb() arguments > > On Thu, Oct 25, 2018 at 06:32:56PM -0700, Dhinakaran Pandiyan wrote: > > Currently there is some duplication of pixel format and modifier > > validation code between the fb creation and plane check paths. We can > > unify them by checking if any plane supports a pixel format and > > modifier combination during framebuffer creation. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 116 > > ++++++----------------------------- > > 1 file changed, 19 insertions(+), 97 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index fe045abb6472..1b5d936a93d0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14362,6 +14362,19 @@ u32 intel_fb_pitch_limit(struct > drm_i915_private *dev_priv, > > DRM_MODE_ROTATE_0); > > } > > > > +static bool any_plane_supports_format(struct drm_i915_private > *dev_priv, > > + uint64_t fb_modifier, > > + uint32_t pixel_format) > > "format first, modifier second" is the typical covention. Yeah, I did go that way. intel_fb_pitch_limit() that's right above inverts the order, so I switched it keep it locally consistent. > > But I think we could stuff this entire thing into the core, in case someone else > wants to reuse it. I think I even posted the patches that do it like that. > Ah yes: https://patchwork.freedesktop.org/series/39700/ Since you posted them first, let's go with it. Patches 1-3 look good to me, can you rebase and send them to the list? The second patch does not apply. > > > +{ > > + struct drm_plane *plane; > > + > > + drm_for_each_plane(plane, &dev_priv->drm) > > + if (!drm_plane_check_pixel_format(plane, pixel_format, > > + fb_modifier)) > > + return true; > > + return false; > > +} > > + > > static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > > struct drm_i915_gem_object *obj, > > struct drm_mode_fb_cmd2 *mode_cmd) > @@ -14399,40 +14412,12 @@ > > 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_Yf_TILED: > > - if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > > - DRM_DEBUG_KMS("Indexed format does not > support Yf tiling\n"); > > - goto err; > > - } > > - /* fall through */ > > - case I915_FORMAT_MOD_Y_TILED: > > - if (INTEL_GEN(dev_priv) < 9) { > > - DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n", > > - mode_cmd->modifier[0]); > > - goto err; > > - } > > - break; > > - 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]); > > + if (!any_plane_supports_format(dev_priv, mode_cmd->modifier[0], > > + mode_cmd->pixel_format)) { > > + DRM_DEBUG_KMS("Unsupported pixel format %s or > modifier 0x%llx\n", > > + drm_get_format_name(mode_cmd- > >pixel_format, > > + &format_name), > > + mode_cmd->modifier[0]); > > goto err; > > } > > > > @@ -14466,69 +14451,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; > > - case DRM_FORMAT_NV12: > > - if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) || > > - IS_BROXTON(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.14.1 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe045abb6472..1b5d936a93d0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14362,6 +14362,19 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv, DRM_MODE_ROTATE_0); } +static bool any_plane_supports_format(struct drm_i915_private *dev_priv, + uint64_t fb_modifier, + uint32_t pixel_format) +{ + struct drm_plane *plane; + + drm_for_each_plane(plane, &dev_priv->drm) + if (!drm_plane_check_pixel_format(plane, pixel_format, + fb_modifier)) + return true; + return false; +} + static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, struct drm_i915_gem_object *obj, struct drm_mode_fb_cmd2 *mode_cmd) @@ -14399,40 +14412,12 @@ 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_Yf_TILED: - if (mode_cmd->pixel_format == DRM_FORMAT_C8) { - DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n"); - goto err; - } - /* fall through */ - case I915_FORMAT_MOD_Y_TILED: - if (INTEL_GEN(dev_priv) < 9) { - DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n", - mode_cmd->modifier[0]); - goto err; - } - break; - 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]); + if (!any_plane_supports_format(dev_priv, mode_cmd->modifier[0], + mode_cmd->pixel_format)) { + DRM_DEBUG_KMS("Unsupported pixel format %s or modifier 0x%llx\n", + drm_get_format_name(mode_cmd->pixel_format, + &format_name), + mode_cmd->modifier[0]); goto err; } @@ -14466,69 +14451,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; - case DRM_FORMAT_NV12: - if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) || - IS_BROXTON(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;
Currently there is some duplication of pixel format and modifier validation code between the fb creation and plane check paths. We can unify them by checking if any plane supports a pixel format and modifier combination during framebuffer creation. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 116 ++++++----------------------------- 1 file changed, 19 insertions(+), 97 deletions(-)