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?
> }
>
> 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 a38d0f20ef2b..ff481af2b24b 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
> @@ -1112,9 +1115,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..0036ca6c1aba 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,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)
> @@ -1844,6 +1814,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;
> }
>
> @@ -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,
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
