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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Remove special case slave handling during hw programming, v3.
Date: Wed, 23 Oct 2019 12:31:43 +0200	[thread overview]
Message-ID: <58094f5c-bab7-76ba-4ddd-8a5e0ac1d451@linux.intel.com> (raw)
In-Reply-To: <20191022181437.GP1208@intel.com>

Op 22-10-2019 om 20:14 schreef Ville Syrjälä:
> On Tue, Oct 22, 2019 at 12:31:49PM +0200, Maarten Lankhorst wrote:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> 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 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 490f09264c7f..e51cbf6b4159 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -11911,6 +11911,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);
>> +		}
> That stuff looks like it deserves a function of its own. Also a bit
> annoying to copy if piecemeal like that. I guess don't have a
> convenient way to just copy the whole thing in one go.
>
> The use of intel_plane_copy_uapi_to_hw_state() also seems wrong
> for the bigjoiner slave case. Shouldn't we just copy the hw state
> and ignore uapi state here entirely?

When using bigjoiner, we copy the props from master_plane_state, then copy the rects from the slave.

We're using uapi.visible/src/dst, it's a tad annoying because there is no separate props struct in drm_plane/crtc_state. Else the distinction would be more clear.

It's very clear in src_x vs src.x1 that there is room for confusion. :(

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