Re: [PATCH 11/18] drm/i915: Move skl plane fb related checks into a better place

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

On Fri, Aug 24, 2018 at 07:56:34PM +0000, Souza, Jose wrote:
> On Thu, 2018-07-19 at 21:22 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Move the skl+ specific framebuffer related checks from
> > intel_plane_atomic_check_with_state() into a new function
> > (skl_plane_check_fb()) which we'll simply call from the skl
> > plane->check() hook.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 42 --------------------
> >  drivers/gpu/drm/i915/intel_display.c      | 12 ------
> >  drivers/gpu/drm/i915/intel_sprite.c       | 66
> > +++++++++++++++++++++++++++++++
> >  3 files changed, 66 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index eddcdd6e4b3b..7c0873060934 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -116,40 +116,11 @@ int intel_plane_atomic_check_with_state(const
> > struct intel_crtc_state *old_crtc_
> >  	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> >  	struct drm_plane_state *state = &intel_state->base;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > -	const struct drm_display_mode *adjusted_mode =
> > -		&crtc_state->base.adjusted_mode;
> >  	int ret;
> >  
> >  	if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> >  		return 0;
> >  
> > -	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> > -		struct drm_format_name_buf format_name;
> > -
> > -		if (state->fb->modifier != I915_FORMAT_MOD_Y_TILED &&
> > -		    state->fb->modifier != I915_FORMAT_MOD_Yf_TILED) {
> > -			DRM_DEBUG_KMS("Y/Yf tiling required for
> > 90/270!\n");
> > -			return -EINVAL;
> > -		}
> > -
> > -		/*
> > -		 * 90/270 is not allowed with RGB64 16:16:16:16,
> > -		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> > -		 * TBD: Add RGB64 case once its added in supported
> > format list.
> > -		 */
> > -		switch (state->fb->format->format) {
> > -		case DRM_FORMAT_C8:
> > -		case DRM_FORMAT_RGB565:
> > -			DRM_DEBUG_KMS("Unsupported pixel format %s for
> > 90/270!\n",
> > -			              drm_get_format_name(state->fb-
> > >format->format,
> > -			                                  &format_name)
> > );
> > -			return -EINVAL;
> > -
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -
> >  	/* CHV ignores the mirror bit when the rotate bit is set :( */
> >  	if (IS_CHERRYVIEW(dev_priv) &&
> >  	    state->rotation & DRM_MODE_ROTATE_180 &&
> > @@ -163,19 +134,6 @@ int intel_plane_atomic_check_with_state(const
> > struct intel_crtc_state *old_crtc_
> >  	if (ret)
> >  		return ret;
> >  
> > -	/*
> > -	 * Y-tiling is not supported in IF-ID Interlace mode in
> > -	 * GEN9 and above.
> > -	 */
> > -	if (state->fb && INTEL_GEN(dev_priv) >= 9 && crtc_state-
> > >base.enable &&
> > -	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> > -		if (state->fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> > -		    state->fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> > -			DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-
> > ID mode\n");
> > -			return -EINVAL;
> > -		}
> > -	}
> > -
> >  	/* FIXME pre-g4x don't work like this */
> >  	if (state->visible)
> >  		crtc_state->active_planes |= BIT(intel_plane->id);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9b9eaeda15dd..2381b762d109 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3151,12 +3151,6 @@ static int skl_check_ccs_aux_surface(struct
> > intel_plane_state *plane_state)
> >  	int y = src_y / vsub;
> >  	u32 offset;
> >  
> > -	if (plane_state->base.rotation & ~(DRM_MODE_ROTATE_0 |
> > DRM_MODE_ROTATE_180)) {
> > -		DRM_DEBUG_KMS("RC support only with 0/180 degree
> > rotation %x\n",
> > -			      plane_state->base.rotation);
> > -		return -EINVAL;
> > -	}
> > -
> >  	intel_add_fb_offsets(&x, &y, plane_state, 1);
> >  	offset = intel_plane_compute_aligned_offset(&x, &y,
> > plane_state, 1);
> >  
> > @@ -3178,12 +3172,6 @@ int skl_check_plane_surface(const struct
> > intel_crtc_state *crtc_state,
> >  	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0,
> > rotation);
> >  	plane_state->color_plane[1].stride = intel_fb_pitch(fb, 1,
> > rotation);
> >  
> > -	if (rotation & DRM_MODE_REFLECT_X &&
> > -	    fb->modifier == DRM_FORMAT_MOD_LINEAR) {
> > -		DRM_DEBUG_KMS("horizontal flip is not supported with
> > linear surface formats\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	if (!plane_state->base.visible)
> >  		return 0;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 36e150885897..041b8921f4fe 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1156,6 +1156,68 @@ vlv_sprite_check(struct intel_crtc_state
> > *crtc_state,
> >  	return 0;
> >  }
> >  
> > +static int skl_plane_check_fb(const struct intel_crtc_state
> > *crtc_state,
> > +			      const struct intel_plane_state
> > *plane_state)
> > +{
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > +	unsigned int rotation = plane_state->base.rotation;
> > +	struct drm_format_name_buf format_name;
> > +
> > +	if (!fb)
> > +		return 0;
> > +
> > +	if (rotation & ~(DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180) &&
> > +	    (fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS &&
> > +	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS)) {
> > +		DRM_DEBUG_KMS("RC support only with 0/180 degree
> > rotation (%x)\n",
> > +			      rotation);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (rotation & DRM_MODE_REFLECT_X &&
> > +	    fb->modifier == DRM_FORMAT_MOD_LINEAR) {
> > +		DRM_DEBUG_KMS("horizontal flip is not supported with
> > linear surface formats\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (drm_rotation_90_or_270(rotation)) {
> > +		if (fb->modifier != I915_FORMAT_MOD_Y_TILED &&
> > +		    fb->modifier != I915_FORMAT_MOD_Yf_TILED) {
> > +			DRM_DEBUG_KMS("Y/Yf tiling required for
> > 90/270!\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		/*
> > +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> > +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> > +		 * TBD: Add RGB64 case once its added in supported
> > format list.
> > +		 */
> > +		switch (fb->format->format) {
> > +		case DRM_FORMAT_C8:
> > +		case DRM_FORMAT_RGB565:
> > +			DRM_DEBUG_KMS("Unsupported pixel format %s for
> > 90/270!\n",
> > +				      drm_get_format_name(fb->format-
> > >format,
> > +							  &format_name)
> > );
> > +			return -EINVAL;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* Y-tiling is not supported in IF-ID Interlace mode */
> > +	if (crtc_state->base.enable &&
> > +	    crtc_state->base.adjusted_mode.flags &
> > DRM_MODE_FLAG_INTERLACE &&
> > +	    (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> > +	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> 
> I guess this 2 checks bellow are new? It was not being handled in
> skl_check_ccs_aux_surface() or else where.

Right. The old code missed the compressed Y tiled cases. Should have
really split that change into a separate patch.

> 
> Other than this everything else looks good.
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > +	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS)) {
> > +		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID
> > mode\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int skl_plane_check(struct intel_crtc_state *crtc_state,
> >  		    struct intel_plane_state *plane_state)
> >  {
> > @@ -1164,6 +1226,10 @@ int skl_plane_check(struct intel_crtc_state
> > *crtc_state,
> >  	int max_scale, min_scale;
> >  	int ret;
> >  
> > +	ret = skl_plane_check_fb(crtc_state, plane_state);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* use scaler when colorkey is not required */
> >  	if (!plane_state->ckey.flags) {
> >  		const struct drm_framebuffer *fb = plane_state-
> > >base.fb;

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