Re: [PATCH 14/14] drm/i915: Remove special case slave handling during hw programming, v2.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 17-10-2019 om 15:21 schreef Maarten Lankhorst:
> 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.
>
> 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   | 55 ++++++-------------
>  5 files changed, 39 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index d9b65e9c45fc..54d112408716 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -309,16 +309,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)
>  {
> @@ -351,25 +341,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 123404a9cf23..726ececd6abd 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 de520d5f1374..88f149cac198 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11768,6 +11768,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;
> +
> +		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 5379b93c2fab..dbca1b3c5c67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -555,6 +555,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
> @@ -1106,9 +1109,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 00f83d37abcf..20123fccd081 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -526,7 +526,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;
> @@ -541,12 +541,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);
>  
> @@ -578,26 +578,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);
> @@ -627,7 +609,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);
> @@ -640,24 +622,13 @@ skl_update_plane(struct intel_plane *plane,
>  {
>  	int color_plane = 0;
>  
> -	if (plane_state->planar_linked_plane) {
> +	if (drm_format_info_is_yuv_semiplanar(plane_state->hw.fb->format) &&
> +	    !plane_state->planar_slave)
>  		/* Program the UV plane */
>  		color_plane = 1;
> -	}
And for pre-icl, this explains the test failures, needs a gen11+ check.
>  
> -	skl_program_plane(plane, crtc_state, plane_state,
> -			  color_plane, false, plane_state->ctl);
> +	skl_program_plane(plane, crtc_state, plane_state, color_plane);
>  }
> -
> -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);
> -}
> -
>  static void
>  skl_disable_plane(struct intel_plane *plane,
>  		  const struct intel_crtc_state *crtc_state)
> @@ -1844,6 +1815,14 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
>  		plane_state->color_ctl = glk_plane_color_ctl(crtc_state,
>  							     plane_state);
>  
> +	if (icl_is_hdr_plane(dev_priv, plane->id) && fb->format->is_yuv)
> +		/* 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;

needs drm_format_info_is_yuv_semiplanar(fb->format) instead of is_yuv check. This explains the test failures on icl+.

>  	return 0;
>  }
>  
> @@ -2512,8 +2491,6 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	plane->disable_plane = skl_disable_plane;
>  	plane->get_hw_state = skl_plane_get_hw_state;
>  	plane->check_plane = skl_plane_check;
> -	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,


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