Re: [PATCH 2/2] drm/i915: Enable 16bpp 90/270 plane rotation for gen10 onwards.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 03, 2017 at 04:37:58PM +0200, Juha-Pekka Heikkila wrote:
> From gen10 onwards 16bpp 90/270 rotation is supported on hardware.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Testcase: https://patchwork.freedesktop.org/patch/186179/
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 6c4c82e2d..502d27a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -107,7 +107,8 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> -static bool intel_valid_rotation(const struct drm_plane_state *state)
> +static bool intel_valid_rotation(const struct drm_plane_state *state,
> +				 const struct drm_i915_private *dev_priv)
>  {

I wouldn't pass the dev_priv. We can dig it out here, eg:

struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

When we do pass dev_priv, it's usually the first parameter to the
function.

>  	struct drm_format_name_buf format_name;
>  
> @@ -118,13 +119,19 @@ static bool intel_valid_rotation(const struct drm_plane_state *state)
>  	}
>  
>  	/*
> -	 * 90/270 is not allowed with RGB64 16:16:16:16,
> -	 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> +	 * 90/270 is not allowed with RGB64 16:16:16:16 and Indexed 8-bit.
> +	 * RGB 16-bit 5:6:5 is allowed gen10 onwards.
>  	 * TBD: Add RGB64 case once its added in supported format list.
>  	 */
>  	switch (state->fb->format->format) {
> -	case DRM_FORMAT_C8:
>  	case DRM_FORMAT_RGB565:
> +		/*
> +		 * gen10 onwards supports 16bpp 90/270 rotation
> +		 */

This comment looks rather redundant.

> +		if (INTEL_GEN(dev_priv) >= 10)
> +			break;
> +

+		/* fall through */

I believe we have -Wimplicit-fallthrough now enabled, and I think that
would warn about this w/o the comment if you have a recent enough gcc.

> +	case DRM_FORMAT_C8:
>  		DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
>  			      drm_get_format_name(state->fb->format->format,
>  						  &format_name));
> @@ -167,7 +174,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
>  	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> -		if (!intel_valid_rotation(state))
> +		if (!intel_valid_rotation(state, dev_priv))
>  			return -EINVAL;
>  	}
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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