Re: [CI 12/12] drm/i915: Remove special case slave handling during hw programming, v3.

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

On Wed, Oct 30, 2019 at 03:26:57PM +0100, Maarten Lankhorst wrote:
> Now that we split plane_state which I didn't want to do yet, we can
> program the slave plane without requiring the master plane.
> 
> This is useful for programming bigjoiner slave planes as well. We
> will no longer need the master's plane_state.
> 
> Changes since v1:
> - set src/dst rectangles after copy_uapi_to_hw_state.
> Changes since v2:
> - Use the correct color_plane for pre-gen11 by using planar_linked_plane != NULL.
> - Use drm_format_info_is_yuv_semiplanar in skl_plane_check() to fix gen11+.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c | 30 +---------
>  .../gpu/drm/i915/display/intel_atomic_plane.h |  3 -
>  drivers/gpu/drm/i915/display/intel_display.c  | 18 ++++++
>  .../drm/i915/display/intel_display_types.h    |  6 +-
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 57 ++++++-------------
>  5 files changed, 40 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 249fb41d78a5..93d391ab3f75 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -348,16 +348,6 @@ void intel_update_plane(struct intel_plane *plane,
>  	plane->update_plane(plane, crtc_state, plane_state);
>  }
>  
> -void intel_update_slave(struct intel_plane *plane,
> -			const struct intel_crtc_state *crtc_state,
> -			const struct intel_plane_state *plane_state)
> -{
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -
> -	trace_intel_update_plane(&plane->base, crtc);
> -	plane->update_slave(plane, crtc_state, plane_state);
> -}
> -
>  void intel_disable_plane(struct intel_plane *plane,
>  			 const struct intel_crtc_state *crtc_state)
>  {
> @@ -390,25 +380,9 @@ void skl_update_planes_on_crtc(struct intel_atomic_state *state,
>  		struct intel_plane_state *new_plane_state =
>  			intel_atomic_get_new_plane_state(state, plane);
>  
> -		if (new_plane_state->uapi.visible) {
> +		if (new_plane_state->uapi.visible ||
> +		    new_plane_state->planar_slave) {
>  			intel_update_plane(plane, new_crtc_state, new_plane_state);
> -		} else if (new_plane_state->planar_slave) {
> -			struct intel_plane *master =
> -				new_plane_state->planar_linked_plane;
> -
> -			/*
> -			 * We update the slave plane from this function because
> -			 * programming it from the master plane's update_plane
> -			 * callback runs into issues when the Y plane is
> -			 * reassigned, disabled or used by a different plane.
> -			 *
> -			 * The slave plane is updated with the master plane's
> -			 * plane_state.
> -			 */
> -			new_plane_state =
> -				intel_atomic_get_new_plane_state(state, master);
> -
> -			intel_update_slave(plane, new_crtc_state, new_plane_state);
>  		} else {
>  			intel_disable_plane(plane, new_crtc_state);
>  		}
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index cdb0f97d09f9..5cedafdddb55 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -25,9 +25,6 @@ void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
>  void intel_update_plane(struct intel_plane *plane,
>  			const struct intel_crtc_state *crtc_state,
>  			const struct intel_plane_state *plane_state);
> -void intel_update_slave(struct intel_plane *plane,
> -			const struct intel_crtc_state *crtc_state,
> -			const struct intel_plane_state *plane_state);
>  void intel_disable_plane(struct intel_plane *plane,
>  			 const struct intel_crtc_state *crtc_state);
>  struct intel_plane *intel_plane_alloc(void);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7ee5d1f5a180..53f9c885fc56 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11988,6 +11988,24 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
>  		crtc_state->active_planes |= BIT(linked->id);
>  		crtc_state->update_planes |= BIT(linked->id);
>  		DRM_DEBUG_KMS("Using %s as Y plane for %s\n", linked->base.name, plane->base.name);
> +
> +		/* Copy parameters to slave plane */
> +		linked_state->ctl = plane_state->ctl | PLANE_CTL_YUV420_Y_PLANE;
> +		linked_state->color_ctl = plane_state->color_ctl;
> +		linked_state->color_plane[0] = plane_state->color_plane[0];
> +
> +		intel_plane_copy_uapi_to_hw_state(linked_state, plane_state);
> +		linked_state->uapi.src = plane_state->uapi.src;
> +		linked_state->uapi.dst = plane_state->uapi.dst;

Still a bit confusing IMO, and I think a blind master.hw -> slave.hw
copy would seem like the more obvious thing do here.
But we can massage this later.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +		if (icl_is_hdr_plane(dev_priv, plane->id)) {
> +			if (linked->id == PLANE_SPRITE5)
> +				plane_state->cus_ctl |= PLANE_CUS_PLANE_7;
> +			else if (linked->id == PLANE_SPRITE4)
> +				plane_state->cus_ctl |= PLANE_CUS_PLANE_6;
> +			else
> +				MISSING_CASE(linked->id);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index fef81e450e22..4a182b62ff2d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -563,6 +563,9 @@ struct intel_plane_state {
>  	/* plane color control register */
>  	u32 color_ctl;
>  
> +	/* chroma upsampler control register */
> +	u32 cus_ctl;
> +
>  	/*
>  	 * scaler_id
>  	 *    = -1 : not using a scaler
> @@ -1122,9 +1125,6 @@ struct intel_plane {
>  	void (*update_plane)(struct intel_plane *plane,
>  			     const struct intel_crtc_state *crtc_state,
>  			     const struct intel_plane_state *plane_state);
> -	void (*update_slave)(struct intel_plane *plane,
> -			     const struct intel_crtc_state *crtc_state,
> -			     const struct intel_plane_state *plane_state);
>  	void (*disable_plane)(struct intel_plane *plane,
>  			      const struct intel_crtc_state *crtc_state);
>  	bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index ef7409f695f9..14b35678a363 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -575,7 +575,7 @@ static void
>  skl_program_plane(struct intel_plane *plane,
>  		  const struct intel_crtc_state *crtc_state,
>  		  const struct intel_plane_state *plane_state,
> -		  int color_plane, bool slave, u32 plane_ctl)
> +		  int color_plane)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum plane_id plane_id = plane->id;
> @@ -590,12 +590,12 @@ skl_program_plane(struct intel_plane *plane,
>  	u32 y = plane_state->color_plane[color_plane].y;
>  	u32 src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
>  	u32 src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> -	struct intel_plane *linked = plane_state->planar_linked_plane;
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	u8 alpha = plane_state->hw.alpha >> 8;
>  	u32 plane_color_ctl = 0;
>  	unsigned long irqflags;
>  	u32 keymsk, keymax;
> +	u32 plane_ctl = plane_state->ctl;
>  
>  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>  
> @@ -627,26 +627,8 @@ skl_program_plane(struct intel_plane *plane,
>  	I915_WRITE_FW(PLANE_AUX_DIST(pipe, plane_id),
>  		      (plane_state->color_plane[1].offset - surf_addr) | aux_stride);
>  
> -	if (icl_is_hdr_plane(dev_priv, plane_id)) {
> -		u32 cus_ctl = 0;
> -
> -		if (linked) {
> -			/* Enable and use MPEG-2 chroma siting */
> -			cus_ctl = PLANE_CUS_ENABLE |
> -				PLANE_CUS_HPHASE_0 |
> -				PLANE_CUS_VPHASE_SIGN_NEGATIVE |
> -				PLANE_CUS_VPHASE_0_25;
> -
> -			if (linked->id == PLANE_SPRITE5)
> -				cus_ctl |= PLANE_CUS_PLANE_7;
> -			else if (linked->id == PLANE_SPRITE4)
> -				cus_ctl |= PLANE_CUS_PLANE_6;
> -			else
> -				MISSING_CASE(linked->id);
> -		}
> -
> -		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
> -	}
> +	if (icl_is_hdr_plane(dev_priv, plane_id))
> +		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), plane_state->cus_ctl);
>  
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
> @@ -676,7 +658,7 @@ skl_program_plane(struct intel_plane *plane,
>  	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
>  		      intel_plane_ggtt_offset(plane_state) + surf_addr);
>  
> -	if (!slave && plane_state->scaler_id >= 0)
> +	if (plane_state->scaler_id >= 0)
>  		skl_program_scaler(plane, crtc_state, plane_state);
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> @@ -689,24 +671,12 @@ skl_update_plane(struct intel_plane *plane,
>  {
>  	int color_plane = 0;
>  
> -	if (plane_state->planar_linked_plane) {
> -		/* Program the UV plane */
> +	if (plane_state->planar_linked_plane && !plane_state->planar_slave)
> +		/* Program the UV plane on planar master */
>  		color_plane = 1;
> -	}
> -
> -	skl_program_plane(plane, crtc_state, plane_state,
> -			  color_plane, false, plane_state->ctl);
> -}
>  
> -static void
> -icl_update_slave(struct intel_plane *plane,
> -		 const struct intel_crtc_state *crtc_state,
> -		 const struct intel_plane_state *plane_state)
> -{
> -	skl_program_plane(plane, crtc_state, plane_state, 0, true,
> -			  plane_state->ctl | PLANE_CTL_YUV420_Y_PLANE);
> +	skl_program_plane(plane, crtc_state, plane_state, color_plane);
>  }
> -
>  static void
>  skl_disable_plane(struct intel_plane *plane,
>  		  const struct intel_crtc_state *crtc_state)
> @@ -2238,6 +2208,15 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
>  		plane_state->color_ctl = glk_plane_color_ctl(crtc_state,
>  							     plane_state);
>  
> +	if (drm_format_info_is_yuv_semiplanar(fb->format) &&
> +	    icl_is_hdr_plane(dev_priv, plane->id))
> +		/* Enable and use MPEG-2 chroma siting */
> +		plane_state->cus_ctl = PLANE_CUS_ENABLE |
> +			PLANE_CUS_HPHASE_0 |
> +			PLANE_CUS_VPHASE_SIGN_NEGATIVE | PLANE_CUS_VPHASE_0_25;
> +	else
> +		plane_state->cus_ctl = 0;
> +
>  	return 0;
>  }
>  
> @@ -2917,8 +2896,6 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	plane->get_hw_state = skl_plane_get_hw_state;
>  	plane->check_plane = skl_plane_check;
>  	plane->min_cdclk = skl_plane_min_cdclk;
> -	if (icl_is_nv12_y_plane(plane_id))
> -		plane->update_slave = icl_update_slave;
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		formats = icl_get_plane_formats(dev_priv, pipe,
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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