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@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
