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.lankho...@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.lankho...@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

Reply via email to