On Fri, Sep 20, 2019 at 01:42:26PM +0200, Maarten Lankhorst wrote:
> We want to program slave planes with the master plane_state for
> properties such as FB, rotation, coordinates, etc, but the
> slave plane_state for all programming parameters.
>
> Instead of special casing NV12 Y-planes, we make the code more
> generic, Y planes are programmed with separate state from the UV
> plane.
>
> This will allow us to program planes on a bigjoiner slave crtc in
> a similar way.
>
> This also requires the VMA to be copied to the slave, which is
> done in prepare_plane_fb().
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
I'm not super wild about how complicated all the plane stuff is
becoming. My hope originally was that we could completely virtualize
our planes and handle uapi and hw state independently for both bigjoiner
and nv12. I.e., make the underlying hardware planes shared resources
that we grab as necessary and setup sort of like how we handle scalers
and dpll's and such elsewhere in the driver. Of course that was just my
super high-level idea and I haven't gone off and done all the design and
implementation you have so I may be completely overlooking problems that
would make that kind of design unworkable.
It's an unfortunate outcome of the increasingly-complicated hardware
design, but I just worry about how hard it's going to be for us to
maintain some of this stuff going forward. I keep confusing myself
while reviewing the patches here, so I imagine it will be even harder
for someone who isn't specifically focused on bigjoiner and nv12 to
unravel and understand the codeflow and state handling that's happening.
When we're all done with this work, I think we're going to need some
really extensive kerneldoc updates that lay out the overall flow of
how/where various bits of plane programming information get pulled from
and at what points in the display update sequence.
Matt
> ---
> .../gpu/drm/i915/display/intel_atomic_plane.c | 21 +++---
> .../gpu/drm/i915/display/intel_atomic_plane.h | 3 -
> drivers/gpu/drm/i915/display/intel_display.c | 56 ++++++++++++--
> drivers/gpu/drm/i915/display/intel_display.h | 3 +-
> .../drm/i915/display/intel_display_types.h | 6 +-
> drivers/gpu/drm/i915/display/intel_sprite.c | 75 ++++++++++---------
> 6 files changed, 106 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index a1a34b9981cc..964db7774d10 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -277,14 +277,15 @@ 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)
> +static void intel_update_slave(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *master_plane_state,
> + const struct intel_plane_state *slave_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);
> + plane->update_slave(plane, crtc_state, master_plane_state, slave_plane_state);
> }
>
> void intel_disable_plane(struct intel_plane *plane,
> @@ -324,6 +325,8 @@ void skl_update_planes_on_crtc(struct intel_atomic_state *state,
> } else if (new_plane_state->planar_slave) {
> struct intel_plane *master =
> new_plane_state->planar_linked_plane;
> + struct intel_plane_state *master_plane_state =
> + intel_atomic_get_new_plane_state(state, master);
>
> /*
> * We update the slave plane from this function because
> @@ -331,13 +334,11 @@ void skl_update_planes_on_crtc(struct intel_atomic_state *state,
> * 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.
> + * The slave plane is updated with the master's
> + * plane_state as extra argument.
> */
> - new_plane_state =
> - intel_atomic_get_new_plane_state(state, master);
> -
> - intel_update_slave(plane, new_crtc_state, new_plane_state);
> + intel_update_slave(plane, new_crtc_state,
> + master_plane_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 cb7ef4f9eafd..33fb85cd3909 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -23,9 +23,6 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_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 0424a378eb51..df588bf47559 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3972,11 +3972,12 @@ static unsigned int skl_plane_stride_mult(const struct drm_framebuffer *fb,
> return intel_tile_width_bytes(fb, color_plane);
> }
>
> -u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> +u32 skl_plane_stride(const struct intel_plane_state *master_plane_state,
> + const struct intel_plane_state *plane_state,
> int color_plane)
> {
> - const struct drm_framebuffer *fb = plane_state->base.fb;
> - unsigned int rotation = plane_state->base.rotation;
> + const struct drm_framebuffer *fb = master_plane_state->base.fb;
> + unsigned int rotation = master_plane_state->base.rotation;
> u32 stride = plane_state->color_plane[color_plane].stride;
>
> if (color_plane >= fb->format->num_planes)
> @@ -11900,6 +11901,23 @@ 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];
> +
> + linked_state->base.src = plane_state->base.src;
> + linked_state->base.dst = plane_state->base.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;
> @@ -14782,6 +14800,23 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> return ret;
> }
>
> + if (to_intel_plane_state(new_state)->planar_slave) {
> + struct intel_plane_state *new_plane_state = to_intel_plane_state(new_state);
> + const struct intel_plane_state *linked_plane_state =
> + intel_atomic_get_new_plane_state(intel_state, new_plane_state->planar_linked_plane);
> +
> + /*
> + * We are a planar slave, VMA is on our planar master,
> + * but may not be the same as the bigjoiner master plane.
> + *
> + * Copy the vma from our planar master and return.
> + */
> + if (linked_plane_state->vma)
> + new_plane_state->vma =
> + i915_vma_get(linked_plane_state->vma);
> + return 0;
> + }
> +
> if (!obj)
> return 0;
>
> @@ -14851,10 +14886,11 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> */
> void
> intel_cleanup_plane_fb(struct drm_plane *plane,
> - struct drm_plane_state *old_state)
> + struct drm_plane_state *_old_state)
> {
> + struct intel_plane_state *old_state = to_intel_plane_state(_old_state);
> struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(old_state->state);
> + to_intel_atomic_state(old_state->base.state);
> struct drm_i915_private *dev_priv = to_i915(plane->dev);
>
> if (intel_state->rps_interactive) {
> @@ -14862,9 +14898,17 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> intel_state->rps_interactive = false;
> }
>
> + /* pin is handled in master plane_state for planar formats */
> + if (old_state->planar_slave) {
> + if (old_state->vma)
> + i915_vma_put(old_state->vma);
> + old_state->vma = NULL;
> + return;
> + }
> +
> /* Should only be called after a successful intel_prepare_plane_fb()! */
> mutex_lock(&dev_priv->drm.struct_mutex);
> - intel_plane_unpin_fb(to_intel_plane_state(old_state));
> + intel_plane_unpin_fb(old_state);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index b1ae0e59c715..764d05d13b9e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -560,7 +560,8 @@ u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
> -u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> +u32 skl_plane_stride(const struct intel_plane_state *master_plane_state,
> + const struct intel_plane_state *plane_state,
> int plane);
> int skl_check_plane_surface(struct intel_plane_state *plane_state);
> int i9xx_check_plane_surface(struct intel_plane_state *plane_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 7e06c61447e6..ace372a76330 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -538,6 +538,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
> @@ -1097,7 +1100,8 @@ struct intel_plane {
> 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);
> + const struct intel_plane_state *master_plane_state,
> + const struct intel_plane_state *slave_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 f0956fecdea4..bca7ff8a8907 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -346,10 +346,11 @@ skl_plane_max_stride(struct intel_plane *plane,
> static void
> skl_program_scaler(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *master_plane_state,
> const struct intel_plane_state *plane_state)
> {
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> - const struct drm_framebuffer *fb = plane_state->base.fb;
> + const struct drm_framebuffer *fb = master_plane_state->base.fb;
> enum pipe pipe = plane->pipe;
> int scaler_id = plane_state->scaler_id;
> const struct intel_scaler *scaler =
> @@ -527,28 +528,29 @@ icl_program_input_csc(struct intel_plane *plane,
> static void
> skl_program_plane(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *master_plane_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;
> enum pipe pipe = plane->pipe;
> - const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> + const struct drm_intel_sprite_colorkey *key = &master_plane_state->ckey;
> u32 surf_addr = plane_state->color_plane[color_plane].offset;
> - u32 stride = skl_plane_stride(plane_state, color_plane);
> - u32 aux_stride = skl_plane_stride(plane_state, 1);
> + u32 stride = skl_plane_stride(master_plane_state, plane_state, color_plane);
> + u32 aux_stride = skl_plane_stride(master_plane_state, plane_state, 1);
> int crtc_x = plane_state->base.dst.x1;
> int crtc_y = plane_state->base.dst.y1;
> u32 x = plane_state->color_plane[color_plane].x;
> u32 y = plane_state->color_plane[color_plane].y;
> u32 src_w = drm_rect_width(&plane_state->base.src) >> 16;
> u32 src_h = drm_rect_height(&plane_state->base.src) >> 16;
> - struct intel_plane *linked = plane_state->planar_linked_plane;
> - const struct drm_framebuffer *fb = plane_state->base.fb;
> - u8 alpha = plane_state->base.alpha >> 8;
> + const struct drm_framebuffer *fb = master_plane_state->base.fb;
> + u8 alpha = master_plane_state->base.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);
>
> @@ -580,32 +582,14 @@ 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);
>
> if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> - icl_program_input_csc(plane, crtc_state, plane_state);
> + icl_program_input_csc(plane, crtc_state, master_plane_state);
>
> skl_write_plane_wm(plane, crtc_state);
>
> @@ -629,8 +613,8 @@ 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)
> - skl_program_scaler(plane, crtc_state, plane_state);
> + if (plane_state->scaler_id >= 0)
> + skl_program_scaler(plane, crtc_state, master_plane_state, plane_state);
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
> @@ -647,17 +631,26 @@ skl_update_plane(struct intel_plane *plane,
> color_plane = 1;
> }
>
> - skl_program_plane(plane, crtc_state, plane_state,
> - color_plane, false, plane_state->ctl);
> + skl_program_plane(plane, crtc_state, plane_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)
> + const struct intel_plane_state *master_plane_state,
> + const struct intel_plane_state *slave_plane_state)
> {
> - skl_program_plane(plane, crtc_state, plane_state, 0, true,
> - plane_state->ctl | PLANE_CTL_YUV420_Y_PLANE);
> + const struct drm_framebuffer *fb = master_plane_state->base.fb;
> + int color_plane = 0;
> +
> + if (drm_format_info_is_yuv_semiplanar(fb->format) &&
> + !slave_plane_state->planar_slave) {
> + /* Program the UV plane, even as slave (Big joiner). */
> + color_plane = 1;
> + }
> +
> + skl_program_plane(plane, crtc_state, master_plane_state,
> + slave_plane_state, color_plane);
> }
>
> static void
> @@ -1845,6 +1838,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;
> +
> return 0;
> }
>
> @@ -2513,7 +2514,7 @@ 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))
> + if (INTEL_GEN(dev_priv) >= 11)
> plane->update_slave = icl_update_slave;
>
> if (INTEL_GEN(dev_priv) >= 11)
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
