[Intel-gfx] [PATCH 2/2] drm/i915: Reuse plane format modifier checks to verify addfb() arguments

[Ville Syrjälä]

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 at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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

-- 
Ville Syrjälä
Intel