| Message ID | 20180921173945.6276-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gen11: Enable planar format support on gen11. | expand |
On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > To make NV12 working on icl, we need to update 2 planes simultaneously. > I've chosen to do this in the CRTC step after plane validation is done, > so we know what planes are (in)visible. The linked Y plane will get > updated in intel_plane_update_planes_on_crtc(), by the call to > update_slave, which gets the master's plane_state as argument. > > The link requires both planes for atomic_update to work, > so make sure skl_ddb_add_affected_planes() adds both states. > > Changes since v1: > - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > - Put all the state updating login in intel_plane_atomic_check_with_state(). > - Clean up changes in intel_plane_atomic_check(). > Changes since v2: > - Fix intel_atomic_get_old_plane_state() to actually return old state. > - Move visibility changes to preparation patch. > - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > fixup Y/UV Linkage > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 12 ++- > 4 files changed, 210 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index 984bc1f26625..522699085a59 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > crtc_state->nv12_planes &= ~BIT(intel_plane->id); > intel_state->base.visible = false; > > - /* If this is a cursor plane, no further checks are needed. */ > + /* Destroy the link */ > + intel_state->linked_plane = NULL; > + intel_state->slave = false; > + > + /* If this is a cursor or Y plane, no further checks are needed. */ > if (!intel_state->base.crtc && !old_plane_state->base.crtc) > return 0; > > @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > state); > } > > -static int intel_plane_atomic_check(struct drm_plane *plane, > - struct drm_plane_state *new_plane_state) > +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > + struct drm_plane_state *new_drm_plane_state) > { > - struct drm_atomic_state *state = new_plane_state->state; > - const struct drm_plane_state *old_plane_state = > - drm_atomic_get_old_plane_state(state, plane); > - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > - const struct drm_crtc_state *old_crtc_state; > - struct drm_crtc_state *new_crtc_state; > - > - new_plane_state->visible = false; > + struct intel_atomic_state *state = > + to_intel_atomic_state(new_drm_plane_state->state); > + struct intel_plane *plane = to_intel_plane(drm_plane); > + const struct intel_plane_state *old_plane_state = > + intel_atomic_get_old_plane_state(state, plane); > + struct intel_plane_state *new_plane_state = > + to_intel_plane_state(new_drm_plane_state); > + struct intel_crtc *crtc = to_intel_crtc( > + new_plane_state->base.crtc ?: > + old_plane_state->base.crtc); > + const struct intel_crtc_state *old_crtc_state; > + struct intel_crtc_state *new_crtc_state; > + struct intel_plane *linked = old_plane_state->linked_plane; > + int ret; > + const struct intel_plane_state *old_linked_state; > + struct intel_plane_state *new_linked_state = NULL; > + > + if (linked) { > + /* > + * Make sure a previously linked plane (and implicitly, the CRTC) > + * is part of the atomic commit. > + */ > + if (!intel_atomic_get_new_plane_state(state, linked)) { > + new_linked_state = intel_atomic_get_plane_state(state, linked); > + if (IS_ERR(new_linked_state)) > + return PTR_ERR(new_linked_state); > + } > + > + old_linked_state = > + intel_atomic_get_old_plane_state(state, linked); > + > + /* > + * This will happen when we're the Y plane. In which case > + * old/new_state->crtc are both NULL. We still need to perform > + * updates on the linked plane. > + */ > + if (!crtc) > + crtc = to_intel_crtc(old_linked_state->base.crtc); > + > + WARN_ON(!crtc); > + } > + > + new_plane_state->base.visible = false; > if (!crtc) > return 0; > > - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > + > + if (new_linked_state && > + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > + /* > + * This function is called from drm_atomic_helper_check_planes(), which > + * will normally check the newly added plane for us, but since we're > + * already in that function, it won't check the plane if our index > + * is bigger than the linked index because of the > + * for_each_oldnew_plane_in_state() call. > + */ > + new_crtc_state->base.planes_changed = true; > + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > + old_linked_state, new_linked_state); > + if (ret) > + return ret; > + } This is all rather confusing. Can't we just do a preprocessing step before check_planes() to add the linked planes as needed, and then let the normal check_planes() do its thing? > > - return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state), > - to_intel_crtc_state(new_crtc_state), > - to_intel_plane_state(old_plane_state), > - to_intel_plane_state(new_plane_state)); > + return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > + old_plane_state, new_plane_state); > } > > void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, > @@ -187,6 +240,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, > trace_intel_update_plane(&plane->base, crtc); > > plane->update_plane(plane, new_crtc_state, new_plane_state); > + } else if (new_plane_state->slave) { > + struct intel_plane *master = > + new_plane_state->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(old_state, master); > + > + trace_intel_update_plane(&plane->base, crtc); > + > + plane->update_slave(plane, new_crtc_state, new_plane_state); > } else { > trace_intel_disable_plane(&plane->base, crtc); > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 390907d77ecd..19cd6bbb43c4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10726,6 +10726,60 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state, > return true; > } > > +static int icl_check_nv12_planes(struct drm_i915_private *dev_priv, > + struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state); > + struct intel_plane *plane, *aux; > + > + if (INTEL_GEN(dev_priv) < 11 || !crtc_state->nv12_planes) > + return 0; > + > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) { > + struct intel_plane_state *plane_state, *aux_state; > + struct drm_plane_state *drm_aux_state = NULL; > + > + if (!(crtc_state->nv12_planes & BIT(plane->id))) > + continue; > + > + plane_state = intel_atomic_get_new_plane_state(state, plane); > + if (!plane_state) > + continue; > + > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) { > + if (!icl_is_nv12_y_plane(aux->id)) > + continue; > + > + if (crtc_state->active_planes & BIT(aux->id)) > + continue; > + > + drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base); > + if (IS_ERR(drm_aux_state)) > + return PTR_ERR(drm_aux_state); > + > + break; > + } > + > + if (!drm_aux_state) { > + DRM_DEBUG_KMS("Need %d free Y planes for NV12\n", > + hweight8(crtc_state->nv12_planes)); > + > + return -EINVAL; > + } > + > + plane_state->linked_plane = aux; > + > + aux_state = to_intel_plane_state(drm_aux_state); Don't really like the aux name here. Each plane can have an aux surface so now having aux planes is getting a bit overloady. You called it slave elsewhere so I'd stick to that everywhere. > + aux_state->slave = true; > + aux_state->linked_plane = plane; > + crtc_state->active_planes |= BIT(aux->id); > + DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name); > + } > + > + return 0; > +} > + > static int intel_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *crtc_state) > { > @@ -10797,6 +10851,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > if (mode_changed) > ret = skl_update_scaler_crtc(pipe_config); > > + if (!ret) > + ret = icl_check_nv12_planes(dev_priv, intel_crtc, > + pipe_config); > if (!ret) > ret = skl_check_pipe_max_pixel_rate(intel_crtc, > pipe_config); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8073a85d7178..29c7a4bb484d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -539,6 +539,26 @@ struct intel_plane_state { > */ > int scaler_id; > > + /* > + * linked_plane: > + * > + * ICL planar formats require 2 planes that are updated as pairs. > + * This member is used to make sure the other plane is also updated > + * when required, and for update_slave() to find the correct > + * plane_state to pass as argument. > + */ > + struct intel_plane *linked_plane; > + > + /* > + * slave: > + * If set don't update use the linked plane's state for updating > + * this plane during atomic commit with the update_slave() callback. > + * > + * It's also used by the watermark code to ignore wm calculations on > + * this plane. They're calculated by the linked plane's wm code. > + */ > + bool slave; Wondering if separate *slave and *master pointers would be make things more obvioius which one we're looking at in each piece of code. > + > struct drm_intel_sprite_colorkey ckey; > }; > > @@ -973,6 +993,9 @@ 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, > struct intel_crtc *crtc); > bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe); > @@ -1330,6 +1353,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) > return container_of(intel_hdmi, struct intel_digital_port, hdmi); > } > > +static inline struct intel_plane_state * > +intel_atomic_get_plane_state(struct intel_atomic_state *state, > + struct intel_plane *plane) > +{ > + struct drm_plane_state *ret = > + drm_atomic_get_plane_state(&state->base, &plane->base); > + > + if (IS_ERR(ret)) > + return ERR_CAST(ret); > + > + return to_intel_plane_state(ret); > +} > + > +static inline struct intel_plane_state * > +intel_atomic_get_old_plane_state(struct intel_atomic_state *state, > + struct intel_plane *plane) > +{ > + return to_intel_plane_state(drm_atomic_get_old_plane_state(&state->base, > + &plane->base)); > +} > + > static inline struct intel_plane_state * > intel_atomic_get_new_plane_state(struct intel_atomic_state *state, > struct intel_plane *plane) > @@ -2143,6 +2187,15 @@ int skl_plane_check(struct intel_crtc_state *crtc_state, > int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state); > int chv_plane_check_rotation(const struct intel_plane_state *plane_state); > > +static inline bool icl_is_nv12_y_plane(enum plane_id id) > +{ > + /* Don't need to do a gen check, these planes are only available on gen11 */ > + if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5) > + return true; > + > + return false; > +} > + > /* intel_tv.c */ > void intel_tv_init(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1db9b8328275..d76d93452137 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5137,11 +5137,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; > struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb; > - struct drm_plane_state *plane_state; > struct drm_plane *plane; > enum pipe pipe = intel_crtc->pipe; > > drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { > + struct drm_plane_state *plane_state; > + struct intel_plane *linked; > enum plane_id plane_id = to_intel_plane(plane)->id; > > if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id], > @@ -5153,6 +5154,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) > return PTR_ERR(plane_state); > + > + /* Make sure linked plane is updated too */ > + linked = to_intel_plane_state(plane_state)->linked_plane; > + if (!linked) > + continue; > + > + plane_state = drm_atomic_get_plane_state(state, &linked->base); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > } > > return 0; > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > > To make NV12 working on icl, we need to update 2 planes simultaneously. > > I've chosen to do this in the CRTC step after plane validation is done, > > so we know what planes are (in)visible. The linked Y plane will get > > updated in intel_plane_update_planes_on_crtc(), by the call to > > update_slave, which gets the master's plane_state as argument. > > > > The link requires both planes for atomic_update to work, > > so make sure skl_ddb_add_affected_planes() adds both states. > > > > Changes since v1: > > - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > > - Put all the state updating login in intel_plane_atomic_check_with_state(). > > - Clean up changes in intel_plane_atomic_check(). > > Changes since v2: > > - Fix intel_atomic_get_old_plane_state() to actually return old state. > > - Move visibility changes to preparation patch. > > - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > fixup Y/UV Linkage > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > > drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 12 ++- > > 4 files changed, 210 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > > index 984bc1f26625..522699085a59 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > > @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > crtc_state->nv12_planes &= ~BIT(intel_plane->id); > > intel_state->base.visible = false; > > > > - /* If this is a cursor plane, no further checks are needed. */ > > + /* Destroy the link */ > > + intel_state->linked_plane = NULL; > > + intel_state->slave = false; > > + > > + /* If this is a cursor or Y plane, no further checks are needed. */ > > if (!intel_state->base.crtc && !old_plane_state->base.crtc) > > return 0; > > > > @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > state); > > } > > > > -static int intel_plane_atomic_check(struct drm_plane *plane, > > - struct drm_plane_state *new_plane_state) > > +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > > + struct drm_plane_state *new_drm_plane_state) > > { > > - struct drm_atomic_state *state = new_plane_state->state; > > - const struct drm_plane_state *old_plane_state = > > - drm_atomic_get_old_plane_state(state, plane); > > - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > > - const struct drm_crtc_state *old_crtc_state; > > - struct drm_crtc_state *new_crtc_state; > > - > > - new_plane_state->visible = false; > > + struct intel_atomic_state *state = > > + to_intel_atomic_state(new_drm_plane_state->state); > > + struct intel_plane *plane = to_intel_plane(drm_plane); > > + const struct intel_plane_state *old_plane_state = > > + intel_atomic_get_old_plane_state(state, plane); > > + struct intel_plane_state *new_plane_state = > > + to_intel_plane_state(new_drm_plane_state); > > + struct intel_crtc *crtc = to_intel_crtc( > > + new_plane_state->base.crtc ?: > > + old_plane_state->base.crtc); > > + const struct intel_crtc_state *old_crtc_state; > > + struct intel_crtc_state *new_crtc_state; > > + struct intel_plane *linked = old_plane_state->linked_plane; > > + int ret; > > + const struct intel_plane_state *old_linked_state; > > + struct intel_plane_state *new_linked_state = NULL; > > + > > + if (linked) { > > + /* > > + * Make sure a previously linked plane (and implicitly, the CRTC) > > + * is part of the atomic commit. > > + */ > > + if (!intel_atomic_get_new_plane_state(state, linked)) { > > + new_linked_state = intel_atomic_get_plane_state(state, linked); > > + if (IS_ERR(new_linked_state)) > > + return PTR_ERR(new_linked_state); > > + } > > + > > + old_linked_state = > > + intel_atomic_get_old_plane_state(state, linked); > > + > > + /* > > + * This will happen when we're the Y plane. In which case > > + * old/new_state->crtc are both NULL. We still need to perform > > + * updates on the linked plane. > > + */ > > + if (!crtc) > > + crtc = to_intel_crtc(old_linked_state->base.crtc); > > + > > + WARN_ON(!crtc); > > + } > > + > > + new_plane_state->base.visible = false; > > if (!crtc) > > return 0; > > > > - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > > - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > + > > + if (new_linked_state && > > + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > > + /* > > + * This function is called from drm_atomic_helper_check_planes(), which > > + * will normally check the newly added plane for us, but since we're > > + * already in that function, it won't check the plane if our index > > + * is bigger than the linked index because of the > > + * for_each_oldnew_plane_in_state() call. > > + */ > > + new_crtc_state->base.planes_changed = true; > > + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > > + old_linked_state, new_linked_state); > > + if (ret) > > + return ret; > > + } > > This is all rather confusing. Can't we just do a preprocessing step > before check_planes() to add the linked planes as needed, and then > let the normal check_planes() do its thing? Also one thing that slightly worries me is what happens if someone adds more planes to the state after this has all been done. I guess currently those cases would either come from the tail end of intel_atomic_check() or from intel_crtc_atomic_check(). Currently it would appear that wm/ddb is the only piece of code that can do this sort of thing. Some ideas how to solve it perhaps: - Move the linked plane pointer to the core so that get_plane_state() can immediately pick up both planes - Add some kind of flag into the top level atomic state that after this point no new planes. But I guess duplicate_state() is the only thing we have and that doesn't know anything about the the top level state - Live with it and tell everyone to be careful? Some kind of intel_add_affected_planes() might be helpful here to at least give people one thing to call in the late stages. Wouldn't help if the call comes from some helper/core function though. And as there is no use for such a function currently no point adding it I suppose. The wm stuff has its own thing, which you covered already. Dunno, maybe I'm just a bit too paranoid here.
On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > To make NV12 working on icl, we need to update 2 planes simultaneously. > I've chosen to do this in the CRTC step after plane validation is done, > so we know what planes are (in)visible. The linked Y plane will get > updated in intel_plane_update_planes_on_crtc(), by the call to > update_slave, which gets the master's plane_state as argument. > > The link requires both planes for atomic_update to work, > so make sure skl_ddb_add_affected_planes() adds both states. Is this paragraph left over from an earlier version of the patch? You don't actually modify skl_ddb_add_affected_planes() here, and you seem to be describing what you actually do in icl_check_nv12_planes(). > > Changes since v1: > - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > - Put all the state updating login in intel_plane_atomic_check_with_state(). > - Clean up changes in intel_plane_atomic_check(). > Changes since v2: > - Fix intel_atomic_get_old_plane_state() to actually return old state. > - Move visibility changes to preparation patch. > - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > fixup Y/UV Linkage > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Squash commit message artifact. > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 12 ++- > 4 files changed, 210 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index 984bc1f26625..522699085a59 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > crtc_state->nv12_planes &= ~BIT(intel_plane->id); > intel_state->base.visible = false; > > - /* If this is a cursor plane, no further checks are needed. */ > + /* Destroy the link */ > + intel_state->linked_plane = NULL; > + intel_state->slave = false; > + > + /* If this is a cursor or Y plane, no further checks are needed. */ > if (!intel_state->base.crtc && !old_plane_state->base.crtc) > return 0; > > @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > state); > } > > -static int intel_plane_atomic_check(struct drm_plane *plane, > - struct drm_plane_state *new_plane_state) > +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > + struct drm_plane_state *new_drm_plane_state) > { > - struct drm_atomic_state *state = new_plane_state->state; > - const struct drm_plane_state *old_plane_state = > - drm_atomic_get_old_plane_state(state, plane); > - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > - const struct drm_crtc_state *old_crtc_state; > - struct drm_crtc_state *new_crtc_state; > - > - new_plane_state->visible = false; > + struct intel_atomic_state *state = > + to_intel_atomic_state(new_drm_plane_state->state); > + struct intel_plane *plane = to_intel_plane(drm_plane); > + const struct intel_plane_state *old_plane_state = > + intel_atomic_get_old_plane_state(state, plane); > + struct intel_plane_state *new_plane_state = > + to_intel_plane_state(new_drm_plane_state); > + struct intel_crtc *crtc = to_intel_crtc( > + new_plane_state->base.crtc ?: > + old_plane_state->base.crtc); > + const struct intel_crtc_state *old_crtc_state; > + struct intel_crtc_state *new_crtc_state; > + struct intel_plane *linked = old_plane_state->linked_plane; > + int ret; > + const struct intel_plane_state *old_linked_state; > + struct intel_plane_state *new_linked_state = NULL; > + > + if (linked) { > + /* > + * Make sure a previously linked plane (and implicitly, the CRTC) > + * is part of the atomic commit. > + */ > + if (!intel_atomic_get_new_plane_state(state, linked)) { > + new_linked_state = intel_atomic_get_plane_state(state, linked); > + if (IS_ERR(new_linked_state)) > + return PTR_ERR(new_linked_state); > + } > + > + old_linked_state = > + intel_atomic_get_old_plane_state(state, linked); > + > + /* > + * This will happen when we're the Y plane. In which case > + * old/new_state->crtc are both NULL. We still need to perform > + * updates on the linked plane. > + */ > + if (!crtc) > + crtc = to_intel_crtc(old_linked_state->base.crtc); > + > + WARN_ON(!crtc); > + } > + > + new_plane_state->base.visible = false; > if (!crtc) > return 0; > > - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > + > + if (new_linked_state && > + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > + /* > + * This function is called from drm_atomic_helper_check_planes(), which > + * will normally check the newly added plane for us, but since we're > + * already in that function, it won't check the plane if our index > + * is bigger than the linked index because of the > + * for_each_oldnew_plane_in_state() call. > + */ > + new_crtc_state->base.planes_changed = true; > + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > + old_linked_state, new_linked_state); > + if (ret) > + return ret; > + } > > - return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state), > - to_intel_crtc_state(new_crtc_state), > - to_intel_plane_state(old_plane_state), > - to_intel_plane_state(new_plane_state)); > + return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > + old_plane_state, new_plane_state); > } > > void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, > @@ -187,6 +240,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, > trace_intel_update_plane(&plane->base, crtc); > > plane->update_plane(plane, new_crtc_state, new_plane_state); > + } else if (new_plane_state->slave) { > + struct intel_plane *master = > + new_plane_state->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(old_state, master); > + > + trace_intel_update_plane(&plane->base, crtc); > + > + plane->update_slave(plane, new_crtc_state, new_plane_state); > } else { > trace_intel_disable_plane(&plane->base, crtc); > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 390907d77ecd..19cd6bbb43c4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10726,6 +10726,60 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state, > return true; > } > > +static int icl_check_nv12_planes(struct drm_i915_private *dev_priv, > + struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state); > + struct intel_plane *plane, *aux; > + > + if (INTEL_GEN(dev_priv) < 11 || !crtc_state->nv12_planes) > + return 0; > + > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) { > + struct intel_plane_state *plane_state, *aux_state; > + struct drm_plane_state *drm_aux_state = NULL; > + > + if (!(crtc_state->nv12_planes & BIT(plane->id))) > + continue; > + > + plane_state = intel_atomic_get_new_plane_state(state, plane); > + if (!plane_state) > + continue; > + > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) { > + if (!icl_is_nv12_y_plane(aux->id)) > + continue; > + > + if (crtc_state->active_planes & BIT(aux->id)) > + continue; > + > + drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base); > + if (IS_ERR(drm_aux_state)) > + return PTR_ERR(drm_aux_state); > + > + break; > + } > + > + if (!drm_aux_state) { > + DRM_DEBUG_KMS("Need %d free Y planes for NV12\n", > + hweight8(crtc_state->nv12_planes)); > + > + return -EINVAL; > + } > + > + plane_state->linked_plane = aux; > + > + aux_state = to_intel_plane_state(drm_aux_state); > + aux_state->slave = true; > + aux_state->linked_plane = plane; > + crtc_state->active_planes |= BIT(aux->id); > + DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name); > + } > + > + return 0; > +} > + > static int intel_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *crtc_state) > { > @@ -10797,6 +10851,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > if (mode_changed) > ret = skl_update_scaler_crtc(pipe_config); > > + if (!ret) > + ret = icl_check_nv12_planes(dev_priv, intel_crtc, > + pipe_config); > if (!ret) > ret = skl_check_pipe_max_pixel_rate(intel_crtc, > pipe_config); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8073a85d7178..29c7a4bb484d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -539,6 +539,26 @@ struct intel_plane_state { > */ > int scaler_id; > > + /* > + * linked_plane: > + * > + * ICL planar formats require 2 planes that are updated as pairs. > + * This member is used to make sure the other plane is also updated > + * when required, and for update_slave() to find the correct > + * plane_state to pass as argument. > + */ > + struct intel_plane *linked_plane; > + > + /* > + * slave: > + * If set don't update use the linked plane's state for updating > + * this plane during atomic commit with the update_slave() callback. > + * > + * It's also used by the watermark code to ignore wm calculations on > + * this plane. They're calculated by the linked plane's wm code. > + */ > + bool slave; While these work fine, I keep expecting 'slave' to be a plane pointer rather than a bool. Would it be easier to just drop the bool and have two plane pointers? struct intel_plane *slave; struct intel_plane *master; That would also make checkpatch quieter, since it doesn't like you using bool's inside of structures. Matt > + > struct drm_intel_sprite_colorkey ckey; > }; > > @@ -973,6 +993,9 @@ 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, > struct intel_crtc *crtc); > bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe); > @@ -1330,6 +1353,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) > return container_of(intel_hdmi, struct intel_digital_port, hdmi); > } > > +static inline struct intel_plane_state * > +intel_atomic_get_plane_state(struct intel_atomic_state *state, > + struct intel_plane *plane) > +{ > + struct drm_plane_state *ret = > + drm_atomic_get_plane_state(&state->base, &plane->base); > + > + if (IS_ERR(ret)) > + return ERR_CAST(ret); > + > + return to_intel_plane_state(ret); > +} > + > +static inline struct intel_plane_state * > +intel_atomic_get_old_plane_state(struct intel_atomic_state *state, > + struct intel_plane *plane) > +{ > + return to_intel_plane_state(drm_atomic_get_old_plane_state(&state->base, > + &plane->base)); > +} > + > static inline struct intel_plane_state * > intel_atomic_get_new_plane_state(struct intel_atomic_state *state, > struct intel_plane *plane) > @@ -2143,6 +2187,15 @@ int skl_plane_check(struct intel_crtc_state *crtc_state, > int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state); > int chv_plane_check_rotation(const struct intel_plane_state *plane_state); > > +static inline bool icl_is_nv12_y_plane(enum plane_id id) > +{ > + /* Don't need to do a gen check, these planes are only available on gen11 */ > + if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5) > + return true; > + > + return false; > +} > + > /* intel_tv.c */ > void intel_tv_init(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1db9b8328275..d76d93452137 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5137,11 +5137,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; > struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb; > - struct drm_plane_state *plane_state; > struct drm_plane *plane; > enum pipe pipe = intel_crtc->pipe; > > drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { > + struct drm_plane_state *plane_state; > + struct intel_plane *linked; > enum plane_id plane_id = to_intel_plane(plane)->id; > > if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id], > @@ -5153,6 +5154,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) > return PTR_ERR(plane_state); > + > + /* Make sure linked plane is updated too */ > + linked = to_intel_plane_state(plane_state)->linked_plane; > + if (!linked) > + continue; > + > + plane_state = drm_atomic_get_plane_state(state, &linked->base); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > } > > return 0; > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 21-09-18 om 21:31 schreef Ville Syrjälä: > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: >>> To make NV12 working on icl, we need to update 2 planes simultaneously. >>> I've chosen to do this in the CRTC step after plane validation is done, >>> so we know what planes are (in)visible. The linked Y plane will get >>> updated in intel_plane_update_planes_on_crtc(), by the call to >>> update_slave, which gets the master's plane_state as argument. >>> >>> The link requires both planes for atomic_update to work, >>> so make sure skl_ddb_add_affected_planes() adds both states. >>> >>> Changes since v1: >>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. >>> - Put all the state updating login in intel_plane_atomic_check_with_state(). >>> - Clean up changes in intel_plane_atomic_check(). >>> Changes since v2: >>> - Fix intel_atomic_get_old_plane_state() to actually return old state. >>> - Move visibility changes to preparation patch. >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> >>> fixup Y/UV Linkage >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- >>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ >>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ >>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- >>> 4 files changed, 210 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >>> index 984bc1f26625..522699085a59 100644 >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ >>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); >>> intel_state->base.visible = false; >>> >>> - /* If this is a cursor plane, no further checks are needed. */ >>> + /* Destroy the link */ >>> + intel_state->linked_plane = NULL; >>> + intel_state->slave = false; >>> + >>> + /* If this is a cursor or Y plane, no further checks are needed. */ >>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) >>> return 0; >>> >>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ >>> state); >>> } >>> >>> -static int intel_plane_atomic_check(struct drm_plane *plane, >>> - struct drm_plane_state *new_plane_state) >>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, >>> + struct drm_plane_state *new_drm_plane_state) >>> { >>> - struct drm_atomic_state *state = new_plane_state->state; >>> - const struct drm_plane_state *old_plane_state = >>> - drm_atomic_get_old_plane_state(state, plane); >>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; >>> - const struct drm_crtc_state *old_crtc_state; >>> - struct drm_crtc_state *new_crtc_state; >>> - >>> - new_plane_state->visible = false; >>> + struct intel_atomic_state *state = >>> + to_intel_atomic_state(new_drm_plane_state->state); >>> + struct intel_plane *plane = to_intel_plane(drm_plane); >>> + const struct intel_plane_state *old_plane_state = >>> + intel_atomic_get_old_plane_state(state, plane); >>> + struct intel_plane_state *new_plane_state = >>> + to_intel_plane_state(new_drm_plane_state); >>> + struct intel_crtc *crtc = to_intel_crtc( >>> + new_plane_state->base.crtc ?: >>> + old_plane_state->base.crtc); >>> + const struct intel_crtc_state *old_crtc_state; >>> + struct intel_crtc_state *new_crtc_state; >>> + struct intel_plane *linked = old_plane_state->linked_plane; >>> + int ret; >>> + const struct intel_plane_state *old_linked_state; >>> + struct intel_plane_state *new_linked_state = NULL; >>> + >>> + if (linked) { >>> + /* >>> + * Make sure a previously linked plane (and implicitly, the CRTC) >>> + * is part of the atomic commit. >>> + */ >>> + if (!intel_atomic_get_new_plane_state(state, linked)) { >>> + new_linked_state = intel_atomic_get_plane_state(state, linked); >>> + if (IS_ERR(new_linked_state)) >>> + return PTR_ERR(new_linked_state); >>> + } >>> + >>> + old_linked_state = >>> + intel_atomic_get_old_plane_state(state, linked); >>> + >>> + /* >>> + * This will happen when we're the Y plane. In which case >>> + * old/new_state->crtc are both NULL. We still need to perform >>> + * updates on the linked plane. >>> + */ >>> + if (!crtc) >>> + crtc = to_intel_crtc(old_linked_state->base.crtc); >>> + >>> + WARN_ON(!crtc); >>> + } >>> + >>> + new_plane_state->base.visible = false; >>> if (!crtc) >>> return 0; >>> >>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); >>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); >>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); >>> + >>> + if (new_linked_state && >>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { >>> + /* >>> + * This function is called from drm_atomic_helper_check_planes(), which >>> + * will normally check the newly added plane for us, but since we're >>> + * already in that function, it won't check the plane if our index >>> + * is bigger than the linked index because of the >>> + * for_each_oldnew_plane_in_state() call. >>> + */ >>> + new_crtc_state->base.planes_changed = true; >>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, >>> + old_linked_state, new_linked_state); >>> + if (ret) >>> + return ret; >>> + } >> This is all rather confusing. Can't we just do a preprocessing step >> before check_planes() to add the linked planes as needed, and then >> let the normal check_planes() do its thing? > Also one thing that slightly worries me is what happens if someone adds > more planes to the state after this has all been done. I guess > currently those cases would either come from the tail end of > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > would appear that wm/ddb is the only piece of code that can do this > sort of thing. I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). Otherwise you can't rerun any validation as required. I've now added a function icl_add_linked_planes helper that iterates over all planes in the state, and adds any linked planes to the transaction. This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked planes are added, without doing it from intel_plane_atomic_check() WM will continue to do its own thing, since it's a design error IMO that it even adds planes to the state to begin with. :) Which is the last option you mentioned below. > Some ideas how to solve it perhaps: > - Move the linked plane pointer to the core so that get_plane_state() > can immediately pick up both planes > - Add some kind of flag into the top level atomic state that after this > point no new planes. But I guess duplicate_state() is the only thing > we have and that doesn't know anything about the the top level state > - Live with it and tell everyone to be careful? Some kind of > intel_add_affected_planes() might be helpful here to at least give > people one thing to call in the late stages. Wouldn't help if the > call comes from some helper/core function though. And as there is > no use for such a function currently no point adding it I suppose. > The wm stuff has its own thing, which you covered already. > > Dunno, maybe I'm just a bit too paranoid here. >
On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: > Op 21-09-18 om 21:31 schreef Ville Syrjälä: > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > >>> To make NV12 working on icl, we need to update 2 planes simultaneously. > >>> I've chosen to do this in the CRTC step after plane validation is done, > >>> so we know what planes are (in)visible. The linked Y plane will get > >>> updated in intel_plane_update_planes_on_crtc(), by the call to > >>> update_slave, which gets the master's plane_state as argument. > >>> > >>> The link requires both planes for atomic_update to work, > >>> so make sure skl_ddb_add_affected_planes() adds both states. > >>> > >>> Changes since v1: > >>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > >>> - Put all the state updating login in intel_plane_atomic_check_with_state(). > >>> - Clean up changes in intel_plane_atomic_check(). > >>> Changes since v2: > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state. > >>> - Move visibility changes to preparation patch. > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > >>> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> > >>> fixup Y/UV Linkage > >>> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > >>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > >>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > >>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- > >>> 4 files changed, 210 insertions(+), 18 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > >>> index 984bc1f26625..522699085a59 100644 > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > >>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); > >>> intel_state->base.visible = false; > >>> > >>> - /* If this is a cursor plane, no further checks are needed. */ > >>> + /* Destroy the link */ > >>> + intel_state->linked_plane = NULL; > >>> + intel_state->slave = false; > >>> + > >>> + /* If this is a cursor or Y plane, no further checks are needed. */ > >>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) > >>> return 0; > >>> > >>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > >>> state); > >>> } > >>> > >>> -static int intel_plane_atomic_check(struct drm_plane *plane, > >>> - struct drm_plane_state *new_plane_state) > >>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > >>> + struct drm_plane_state *new_drm_plane_state) > >>> { > >>> - struct drm_atomic_state *state = new_plane_state->state; > >>> - const struct drm_plane_state *old_plane_state = > >>> - drm_atomic_get_old_plane_state(state, plane); > >>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > >>> - const struct drm_crtc_state *old_crtc_state; > >>> - struct drm_crtc_state *new_crtc_state; > >>> - > >>> - new_plane_state->visible = false; > >>> + struct intel_atomic_state *state = > >>> + to_intel_atomic_state(new_drm_plane_state->state); > >>> + struct intel_plane *plane = to_intel_plane(drm_plane); > >>> + const struct intel_plane_state *old_plane_state = > >>> + intel_atomic_get_old_plane_state(state, plane); > >>> + struct intel_plane_state *new_plane_state = > >>> + to_intel_plane_state(new_drm_plane_state); > >>> + struct intel_crtc *crtc = to_intel_crtc( > >>> + new_plane_state->base.crtc ?: > >>> + old_plane_state->base.crtc); > >>> + const struct intel_crtc_state *old_crtc_state; > >>> + struct intel_crtc_state *new_crtc_state; > >>> + struct intel_plane *linked = old_plane_state->linked_plane; > >>> + int ret; > >>> + const struct intel_plane_state *old_linked_state; > >>> + struct intel_plane_state *new_linked_state = NULL; > >>> + > >>> + if (linked) { > >>> + /* > >>> + * Make sure a previously linked plane (and implicitly, the CRTC) > >>> + * is part of the atomic commit. > >>> + */ > >>> + if (!intel_atomic_get_new_plane_state(state, linked)) { > >>> + new_linked_state = intel_atomic_get_plane_state(state, linked); > >>> + if (IS_ERR(new_linked_state)) > >>> + return PTR_ERR(new_linked_state); > >>> + } > >>> + > >>> + old_linked_state = > >>> + intel_atomic_get_old_plane_state(state, linked); > >>> + > >>> + /* > >>> + * This will happen when we're the Y plane. In which case > >>> + * old/new_state->crtc are both NULL. We still need to perform > >>> + * updates on the linked plane. > >>> + */ > >>> + if (!crtc) > >>> + crtc = to_intel_crtc(old_linked_state->base.crtc); > >>> + > >>> + WARN_ON(!crtc); > >>> + } > >>> + > >>> + new_plane_state->base.visible = false; > >>> if (!crtc) > >>> return 0; > >>> > >>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > >>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > >>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > >>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > >>> + > >>> + if (new_linked_state && > >>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > >>> + /* > >>> + * This function is called from drm_atomic_helper_check_planes(), which > >>> + * will normally check the newly added plane for us, but since we're > >>> + * already in that function, it won't check the plane if our index > >>> + * is bigger than the linked index because of the > >>> + * for_each_oldnew_plane_in_state() call. > >>> + */ > >>> + new_crtc_state->base.planes_changed = true; > >>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > >>> + old_linked_state, new_linked_state); > >>> + if (ret) > >>> + return ret; > >>> + } > >> This is all rather confusing. Can't we just do a preprocessing step > >> before check_planes() to add the linked planes as needed, and then > >> let the normal check_planes() do its thing? > > Also one thing that slightly worries me is what happens if someone adds > > more planes to the state after this has all been done. I guess > > currently those cases would either come from the tail end of > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > > would appear that wm/ddb is the only piece of code that can do this > > sort of thing. > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). > Otherwise you can't rerun any validation as required. You shouldn't need validation for eg. dpms on/off. I guess we currently do that although we shouldn't have to. > > I've now added a function icl_add_linked_planes helper that iterates over all planes in > the state, and adds any linked planes to the transaction. > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked > planes are added, without doing it from intel_plane_atomic_check() > > WM will continue to do its own thing, since it's a design error IMO that it even adds > planes to the state to begin with. :) It pretty much has to. The design error we have at the moment is not programming the watermarks from the update_plane()/disable_plane(). That one I've attempted to fix in: git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence And supposedly that one does fix bugs related to watermarks vs. plane updates. > > Which is the last option you mentioned below. > > > Some ideas how to solve it perhaps: > > - Move the linked plane pointer to the core so that get_plane_state() > > can immediately pick up both planes > > - Add some kind of flag into the top level atomic state that after this > > point no new planes. But I guess duplicate_state() is the only thing > > we have and that doesn't know anything about the the top level state > > - Live with it and tell everyone to be careful? Some kind of > > intel_add_affected_planes() might be helpful here to at least give > > people one thing to call in the late stages. Wouldn't help if the > > call comes from some helper/core function though. And as there is > > no use for such a function currently no point adding it I suppose. > > The wm stuff has its own thing, which you covered already. > > > > Dunno, maybe I'm just a bit too paranoid here. > >
Op 24-09-18 om 15:18 schreef Ville Syrjälä: > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: >> Op 21-09-18 om 21:31 schreef Ville Syrjälä: >>> On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: >>>> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: >>>>> To make NV12 working on icl, we need to update 2 planes simultaneously. >>>>> I've chosen to do this in the CRTC step after plane validation is done, >>>>> so we know what planes are (in)visible. The linked Y plane will get >>>>> updated in intel_plane_update_planes_on_crtc(), by the call to >>>>> update_slave, which gets the master's plane_state as argument. >>>>> >>>>> The link requires both planes for atomic_update to work, >>>>> so make sure skl_ddb_add_affected_planes() adds both states. >>>>> >>>>> Changes since v1: >>>>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. >>>>> - Put all the state updating login in intel_plane_atomic_check_with_state(). >>>>> - Clean up changes in intel_plane_atomic_check(). >>>>> Changes since v2: >>>>> - Fix intel_atomic_get_old_plane_state() to actually return old state. >>>>> - Move visibility changes to preparation patch. >>>>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. >>>>> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> >>>>> fixup Y/UV Linkage >>>>> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- >>>>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ >>>>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ >>>>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- >>>>> 4 files changed, 210 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >>>>> index 984bc1f26625..522699085a59 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >>>>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ >>>>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); >>>>> intel_state->base.visible = false; >>>>> >>>>> - /* If this is a cursor plane, no further checks are needed. */ >>>>> + /* Destroy the link */ >>>>> + intel_state->linked_plane = NULL; >>>>> + intel_state->slave = false; >>>>> + >>>>> + /* If this is a cursor or Y plane, no further checks are needed. */ >>>>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) >>>>> return 0; >>>>> >>>>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ >>>>> state); >>>>> } >>>>> >>>>> -static int intel_plane_atomic_check(struct drm_plane *plane, >>>>> - struct drm_plane_state *new_plane_state) >>>>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, >>>>> + struct drm_plane_state *new_drm_plane_state) >>>>> { >>>>> - struct drm_atomic_state *state = new_plane_state->state; >>>>> - const struct drm_plane_state *old_plane_state = >>>>> - drm_atomic_get_old_plane_state(state, plane); >>>>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; >>>>> - const struct drm_crtc_state *old_crtc_state; >>>>> - struct drm_crtc_state *new_crtc_state; >>>>> - >>>>> - new_plane_state->visible = false; >>>>> + struct intel_atomic_state *state = >>>>> + to_intel_atomic_state(new_drm_plane_state->state); >>>>> + struct intel_plane *plane = to_intel_plane(drm_plane); >>>>> + const struct intel_plane_state *old_plane_state = >>>>> + intel_atomic_get_old_plane_state(state, plane); >>>>> + struct intel_plane_state *new_plane_state = >>>>> + to_intel_plane_state(new_drm_plane_state); >>>>> + struct intel_crtc *crtc = to_intel_crtc( >>>>> + new_plane_state->base.crtc ?: >>>>> + old_plane_state->base.crtc); >>>>> + const struct intel_crtc_state *old_crtc_state; >>>>> + struct intel_crtc_state *new_crtc_state; >>>>> + struct intel_plane *linked = old_plane_state->linked_plane; >>>>> + int ret; >>>>> + const struct intel_plane_state *old_linked_state; >>>>> + struct intel_plane_state *new_linked_state = NULL; >>>>> + >>>>> + if (linked) { >>>>> + /* >>>>> + * Make sure a previously linked plane (and implicitly, the CRTC) >>>>> + * is part of the atomic commit. >>>>> + */ >>>>> + if (!intel_atomic_get_new_plane_state(state, linked)) { >>>>> + new_linked_state = intel_atomic_get_plane_state(state, linked); >>>>> + if (IS_ERR(new_linked_state)) >>>>> + return PTR_ERR(new_linked_state); >>>>> + } >>>>> + >>>>> + old_linked_state = >>>>> + intel_atomic_get_old_plane_state(state, linked); >>>>> + >>>>> + /* >>>>> + * This will happen when we're the Y plane. In which case >>>>> + * old/new_state->crtc are both NULL. We still need to perform >>>>> + * updates on the linked plane. >>>>> + */ >>>>> + if (!crtc) >>>>> + crtc = to_intel_crtc(old_linked_state->base.crtc); >>>>> + >>>>> + WARN_ON(!crtc); >>>>> + } >>>>> + >>>>> + new_plane_state->base.visible = false; >>>>> if (!crtc) >>>>> return 0; >>>>> >>>>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); >>>>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); >>>>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); >>>>> + >>>>> + if (new_linked_state && >>>>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { >>>>> + /* >>>>> + * This function is called from drm_atomic_helper_check_planes(), which >>>>> + * will normally check the newly added plane for us, but since we're >>>>> + * already in that function, it won't check the plane if our index >>>>> + * is bigger than the linked index because of the >>>>> + * for_each_oldnew_plane_in_state() call. >>>>> + */ >>>>> + new_crtc_state->base.planes_changed = true; >>>>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, >>>>> + old_linked_state, new_linked_state); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>> This is all rather confusing. Can't we just do a preprocessing step >>>> before check_planes() to add the linked planes as needed, and then >>>> let the normal check_planes() do its thing? >>> Also one thing that slightly worries me is what happens if someone adds >>> more planes to the state after this has all been done. I guess >>> currently those cases would either come from the tail end of >>> intel_atomic_check() or from intel_crtc_atomic_check(). Currently it >>> would appear that wm/ddb is the only piece of code that can do this >>> sort of thing. >> I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). >> Otherwise you can't rerun any validation as required. > You shouldn't need validation for eg. dpms on/off. I guess we currently > do that although we shouldn't have to. We should, if we ever have 2 crtc's active and disable 1. Watermarks should be distributed over active planes only, which dpms toggle affects. Only way around setting plane_state->visible when plane is inactive and calculate minimum requirements, then calculate max requirements. We would have to fix up all of wm programming and plane programming to make it work. I don't think the extra complexity is worth the effort, tbh.. >> I've now added a function icl_add_linked_planes helper that iterates over all planes in >> the state, and adds any linked planes to the transaction. >> >> This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked >> planes are added, without doing it from intel_plane_atomic_check() >> >> WM will continue to do its own thing, since it's a design error IMO that it even adds >> planes to the state to begin with. :) > It pretty much has to. The design error we have at the moment is not > programming the watermarks from the update_plane()/disable_plane(). > That one I've attempted to fix in: > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > And supposedly that one does fix bugs related to watermarks vs. > plane updates. Yeah, fortunately it shouldn't affect this code much, should be easy to rebase. :) Though I would like to get rid of skl_next_plane_to_commit, ugh.. Which is probably a confirmation that the nv12 gen11 series isn't making plane programming that much more complicated, fortunately. I would really like to simplify the locking first at some point. It can't be good to sync write everything. each plane update now does: spin_lock() I915_WRITE_FW(...) POSTING_READ() spin_unlock() Would be nice if we ~Maarten
On Tue, Sep 25, 2018 at 12:03:47PM +0200, Maarten Lankhorst wrote: > Op 24-09-18 om 15:18 schreef Ville Syrjälä: > > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: > >> Op 21-09-18 om 21:31 schreef Ville Syrjälä: > >>> On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > >>>> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > >>>>> To make NV12 working on icl, we need to update 2 planes simultaneously. > >>>>> I've chosen to do this in the CRTC step after plane validation is done, > >>>>> so we know what planes are (in)visible. The linked Y plane will get > >>>>> updated in intel_plane_update_planes_on_crtc(), by the call to > >>>>> update_slave, which gets the master's plane_state as argument. > >>>>> > >>>>> The link requires both planes for atomic_update to work, > >>>>> so make sure skl_ddb_add_affected_planes() adds both states. > >>>>> > >>>>> Changes since v1: > >>>>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > >>>>> - Put all the state updating login in intel_plane_atomic_check_with_state(). > >>>>> - Clean up changes in intel_plane_atomic_check(). > >>>>> Changes since v2: > >>>>> - Fix intel_atomic_get_old_plane_state() to actually return old state. > >>>>> - Move visibility changes to preparation patch. > >>>>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > >>>>> > >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> > >>>>> fixup Y/UV Linkage > >>>>> > >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> --- > >>>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > >>>>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > >>>>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > >>>>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- > >>>>> 4 files changed, 210 insertions(+), 18 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > >>>>> index 984bc1f26625..522699085a59 100644 > >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > >>>>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > >>>>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); > >>>>> intel_state->base.visible = false; > >>>>> > >>>>> - /* If this is a cursor plane, no further checks are needed. */ > >>>>> + /* Destroy the link */ > >>>>> + intel_state->linked_plane = NULL; > >>>>> + intel_state->slave = false; > >>>>> + > >>>>> + /* If this is a cursor or Y plane, no further checks are needed. */ > >>>>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) > >>>>> return 0; > >>>>> > >>>>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > >>>>> state); > >>>>> } > >>>>> > >>>>> -static int intel_plane_atomic_check(struct drm_plane *plane, > >>>>> - struct drm_plane_state *new_plane_state) > >>>>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > >>>>> + struct drm_plane_state *new_drm_plane_state) > >>>>> { > >>>>> - struct drm_atomic_state *state = new_plane_state->state; > >>>>> - const struct drm_plane_state *old_plane_state = > >>>>> - drm_atomic_get_old_plane_state(state, plane); > >>>>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > >>>>> - const struct drm_crtc_state *old_crtc_state; > >>>>> - struct drm_crtc_state *new_crtc_state; > >>>>> - > >>>>> - new_plane_state->visible = false; > >>>>> + struct intel_atomic_state *state = > >>>>> + to_intel_atomic_state(new_drm_plane_state->state); > >>>>> + struct intel_plane *plane = to_intel_plane(drm_plane); > >>>>> + const struct intel_plane_state *old_plane_state = > >>>>> + intel_atomic_get_old_plane_state(state, plane); > >>>>> + struct intel_plane_state *new_plane_state = > >>>>> + to_intel_plane_state(new_drm_plane_state); > >>>>> + struct intel_crtc *crtc = to_intel_crtc( > >>>>> + new_plane_state->base.crtc ?: > >>>>> + old_plane_state->base.crtc); > >>>>> + const struct intel_crtc_state *old_crtc_state; > >>>>> + struct intel_crtc_state *new_crtc_state; > >>>>> + struct intel_plane *linked = old_plane_state->linked_plane; > >>>>> + int ret; > >>>>> + const struct intel_plane_state *old_linked_state; > >>>>> + struct intel_plane_state *new_linked_state = NULL; > >>>>> + > >>>>> + if (linked) { > >>>>> + /* > >>>>> + * Make sure a previously linked plane (and implicitly, the CRTC) > >>>>> + * is part of the atomic commit. > >>>>> + */ > >>>>> + if (!intel_atomic_get_new_plane_state(state, linked)) { > >>>>> + new_linked_state = intel_atomic_get_plane_state(state, linked); > >>>>> + if (IS_ERR(new_linked_state)) > >>>>> + return PTR_ERR(new_linked_state); > >>>>> + } > >>>>> + > >>>>> + old_linked_state = > >>>>> + intel_atomic_get_old_plane_state(state, linked); > >>>>> + > >>>>> + /* > >>>>> + * This will happen when we're the Y plane. In which case > >>>>> + * old/new_state->crtc are both NULL. We still need to perform > >>>>> + * updates on the linked plane. > >>>>> + */ > >>>>> + if (!crtc) > >>>>> + crtc = to_intel_crtc(old_linked_state->base.crtc); > >>>>> + > >>>>> + WARN_ON(!crtc); > >>>>> + } > >>>>> + > >>>>> + new_plane_state->base.visible = false; > >>>>> if (!crtc) > >>>>> return 0; > >>>>> > >>>>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > >>>>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > >>>>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > >>>>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > >>>>> + > >>>>> + if (new_linked_state && > >>>>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > >>>>> + /* > >>>>> + * This function is called from drm_atomic_helper_check_planes(), which > >>>>> + * will normally check the newly added plane for us, but since we're > >>>>> + * already in that function, it won't check the plane if our index > >>>>> + * is bigger than the linked index because of the > >>>>> + * for_each_oldnew_plane_in_state() call. > >>>>> + */ > >>>>> + new_crtc_state->base.planes_changed = true; > >>>>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > >>>>> + old_linked_state, new_linked_state); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + } > >>>> This is all rather confusing. Can't we just do a preprocessing step > >>>> before check_planes() to add the linked planes as needed, and then > >>>> let the normal check_planes() do its thing? > >>> Also one thing that slightly worries me is what happens if someone adds > >>> more planes to the state after this has all been done. I guess > >>> currently those cases would either come from the tail end of > >>> intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > >>> would appear that wm/ddb is the only piece of code that can do this > >>> sort of thing. > >> I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). > >> Otherwise you can't rerun any validation as required. > > You shouldn't need validation for eg. dpms on/off. I guess we currently > > do that although we shouldn't have to. > We should, if we ever have 2 crtc's active and disable 1. Watermarks should be > distributed over active planes only, which dpms toggle affects. I think the rule we want aim for is .enable controls watermarks, .active does not matter. That will guarantee that dpms on will never fail on account of watermarks. The current way of doing things is IMO just wrong and we should eventually fix it. > Only way around setting > plane_state->visible when plane is inactive and calculate minimum requirements, then > calculate max requirements. > > We would have to fix up all of wm programming and plane programming to make it work. > > I don't think the extra complexity is worth the effort, tbh.. > >> I've now added a function icl_add_linked_planes helper that iterates over all planes in > >> the state, and adds any linked planes to the transaction. > >> > >> This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked > >> planes are added, without doing it from intel_plane_atomic_check() > >> > >> WM will continue to do its own thing, since it's a design error IMO that it even adds > >> planes to the state to begin with. :) > > It pretty much has to. The design error we have at the moment is not > > programming the watermarks from the update_plane()/disable_plane(). > > That one I've attempted to fix in: > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > > > And supposedly that one does fix bugs related to watermarks vs. > > plane updates. > > Yeah, fortunately it shouldn't affect this code much, should be easy to rebase. :) > Though I would like to get rid of skl_next_plane_to_commit, ugh.. That thing is pretty much whole point of the series. It could of course be written in a different way, but the idea of ddb allocations driving the plane commit order is key. > > Which is probably a confirmation that the nv12 gen11 series isn't making plane programming > that much more complicated, fortunately. > > I would really like to simplify the locking first at some point. It can't be good to sync write everything. > > each plane update now does: > > spin_lock() > I915_WRITE_FW(...) > > POSTING_READ() Posting reads we should just nuke I think. > spin_unlock() > > Would be nice if we Cut off, but I'll assume you wanted to suggest grabbing the lock just once for the entire pipe commit. I do agree that it would probably be nicer. But I still maintain that splitting the lock to display engine vs. "the rest" is probably something we want to do as well. The annoying thing with that was that I couldn't trick the compiler select the right lock at compile time, so I think this would involve splitting I915_READ/WRITE & co. for display engine vs. the rest as well. I have a feeling I even typed up something like that at one point, just never had the time to test it.
On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote: > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: > > Op 21-09-18 om 21:31 schreef Ville Syrjälä: > > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > > >>> To make NV12 working on icl, we need to update 2 planes simultaneously. > > >>> I've chosen to do this in the CRTC step after plane validation is done, > > >>> so we know what planes are (in)visible. The linked Y plane will get > > >>> updated in intel_plane_update_planes_on_crtc(), by the call to > > >>> update_slave, which gets the master's plane_state as argument. > > >>> > > >>> The link requires both planes for atomic_update to work, > > >>> so make sure skl_ddb_add_affected_planes() adds both states. > > >>> > > >>> Changes since v1: > > >>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > > >>> - Put all the state updating login in intel_plane_atomic_check_with_state(). > > >>> - Clean up changes in intel_plane_atomic_check(). > > >>> Changes since v2: > > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state. > > >>> - Move visibility changes to preparation patch. > > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > >>> > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >>> > > >>> fixup Y/UV Linkage > > >>> > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >>> --- > > >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > > >>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > > >>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > > >>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- > > >>> 4 files changed, 210 insertions(+), 18 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > > >>> index 984bc1f26625..522699085a59 100644 > > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > >>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); > > >>> intel_state->base.visible = false; > > >>> > > >>> - /* If this is a cursor plane, no further checks are needed. */ > > >>> + /* Destroy the link */ > > >>> + intel_state->linked_plane = NULL; > > >>> + intel_state->slave = false; > > >>> + > > >>> + /* If this is a cursor or Y plane, no further checks are needed. */ > > >>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) > > >>> return 0; > > >>> > > >>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > >>> state); > > >>> } > > >>> > > >>> -static int intel_plane_atomic_check(struct drm_plane *plane, > > >>> - struct drm_plane_state *new_plane_state) > > >>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > > >>> + struct drm_plane_state *new_drm_plane_state) > > >>> { > > >>> - struct drm_atomic_state *state = new_plane_state->state; > > >>> - const struct drm_plane_state *old_plane_state = > > >>> - drm_atomic_get_old_plane_state(state, plane); > > >>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > > >>> - const struct drm_crtc_state *old_crtc_state; > > >>> - struct drm_crtc_state *new_crtc_state; > > >>> - > > >>> - new_plane_state->visible = false; > > >>> + struct intel_atomic_state *state = > > >>> + to_intel_atomic_state(new_drm_plane_state->state); > > >>> + struct intel_plane *plane = to_intel_plane(drm_plane); > > >>> + const struct intel_plane_state *old_plane_state = > > >>> + intel_atomic_get_old_plane_state(state, plane); > > >>> + struct intel_plane_state *new_plane_state = > > >>> + to_intel_plane_state(new_drm_plane_state); > > >>> + struct intel_crtc *crtc = to_intel_crtc( > > >>> + new_plane_state->base.crtc ?: > > >>> + old_plane_state->base.crtc); > > >>> + const struct intel_crtc_state *old_crtc_state; > > >>> + struct intel_crtc_state *new_crtc_state; > > >>> + struct intel_plane *linked = old_plane_state->linked_plane; > > >>> + int ret; > > >>> + const struct intel_plane_state *old_linked_state; > > >>> + struct intel_plane_state *new_linked_state = NULL; > > >>> + > > >>> + if (linked) { > > >>> + /* > > >>> + * Make sure a previously linked plane (and implicitly, the CRTC) > > >>> + * is part of the atomic commit. > > >>> + */ > > >>> + if (!intel_atomic_get_new_plane_state(state, linked)) { > > >>> + new_linked_state = intel_atomic_get_plane_state(state, linked); > > >>> + if (IS_ERR(new_linked_state)) > > >>> + return PTR_ERR(new_linked_state); > > >>> + } > > >>> + > > >>> + old_linked_state = > > >>> + intel_atomic_get_old_plane_state(state, linked); > > >>> + > > >>> + /* > > >>> + * This will happen when we're the Y plane. In which case > > >>> + * old/new_state->crtc are both NULL. We still need to perform > > >>> + * updates on the linked plane. > > >>> + */ > > >>> + if (!crtc) > > >>> + crtc = to_intel_crtc(old_linked_state->base.crtc); > > >>> + > > >>> + WARN_ON(!crtc); > > >>> + } > > >>> + > > >>> + new_plane_state->base.visible = false; > > >>> if (!crtc) > > >>> return 0; > > >>> > > >>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > > >>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > >>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > >>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > >>> + > > >>> + if (new_linked_state && > > >>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > > >>> + /* > > >>> + * This function is called from drm_atomic_helper_check_planes(), which > > >>> + * will normally check the newly added plane for us, but since we're > > >>> + * already in that function, it won't check the plane if our index > > >>> + * is bigger than the linked index because of the > > >>> + * for_each_oldnew_plane_in_state() call. > > >>> + */ > > >>> + new_crtc_state->base.planes_changed = true; > > >>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > > >>> + old_linked_state, new_linked_state); > > >>> + if (ret) > > >>> + return ret; > > >>> + } > > >> This is all rather confusing. Can't we just do a preprocessing step > > >> before check_planes() to add the linked planes as needed, and then > > >> let the normal check_planes() do its thing? > > > Also one thing that slightly worries me is what happens if someone adds > > > more planes to the state after this has all been done. I guess > > > currently those cases would either come from the tail end of > > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > > > would appear that wm/ddb is the only piece of code that can do this > > > sort of thing. > > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). > > Otherwise you can't rerun any validation as required. > > You shouldn't need validation for eg. dpms on/off. I guess we currently > do that although we shouldn't have to. > > > > > I've now added a function icl_add_linked_planes helper that iterates over all planes in > > the state, and adds any linked planes to the transaction. > > > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked > > planes are added, without doing it from intel_plane_atomic_check() > > > > WM will continue to do its own thing, since it's a design error IMO that it even adds > > planes to the state to begin with. :) > > It pretty much has to. The design error we have at the moment is not > programming the watermarks from the update_plane()/disable_plane(). > That one I've attempted to fix in: > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > And supposedly that one does fix bugs related to watermarks vs. > plane updates. Where did the bugs arise? Were we unsuccessful at actually evading the vblank (leading to the planes and watermarks taking effect on different vblanks) or is it something else? Matt > > > > > Which is the last option you mentioned below. > > > > > Some ideas how to solve it perhaps: > > > - Move the linked plane pointer to the core so that get_plane_state() > > > can immediately pick up both planes > > > - Add some kind of flag into the top level atomic state that after this > > > point no new planes. But I guess duplicate_state() is the only thing > > > we have and that doesn't know anything about the the top level state > > > - Live with it and tell everyone to be careful? Some kind of > > > intel_add_affected_planes() might be helpful here to at least give > > > people one thing to call in the late stages. Wouldn't help if the > > > call comes from some helper/core function though. And as there is > > > no use for such a function currently no point adding it I suppose. > > > The wm stuff has its own thing, which you covered already. > > > > > > Dunno, maybe I'm just a bit too paranoid here. > > > > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote: > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote: > > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: > > > Op 21-09-18 om 21:31 schreef Ville Syrjälä: > > > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > > > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > > > >>> To make NV12 working on icl, we need to update 2 planes simultaneously. > > > >>> I've chosen to do this in the CRTC step after plane validation is done, > > > >>> so we know what planes are (in)visible. The linked Y plane will get > > > >>> updated in intel_plane_update_planes_on_crtc(), by the call to > > > >>> update_slave, which gets the master's plane_state as argument. > > > >>> > > > >>> The link requires both planes for atomic_update to work, > > > >>> so make sure skl_ddb_add_affected_planes() adds both states. > > > >>> > > > >>> Changes since v1: > > > >>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > > > >>> - Put all the state updating login in intel_plane_atomic_check_with_state(). > > > >>> - Clean up changes in intel_plane_atomic_check(). > > > >>> Changes since v2: > > > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state. > > > >>> - Move visibility changes to preparation patch. > > > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > > >>> > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > >>> > > > >>> fixup Y/UV Linkage > > > >>> > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > >>> --- > > > >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > > > >>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > > > >>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > > > >>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- > > > >>> 4 files changed, 210 insertions(+), 18 deletions(-) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > > > >>> index 984bc1f26625..522699085a59 100644 > > > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > > > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > > > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > > >>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); > > > >>> intel_state->base.visible = false; > > > >>> > > > >>> - /* If this is a cursor plane, no further checks are needed. */ > > > >>> + /* Destroy the link */ > > > >>> + intel_state->linked_plane = NULL; > > > >>> + intel_state->slave = false; > > > >>> + > > > >>> + /* If this is a cursor or Y plane, no further checks are needed. */ > > > >>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) > > > >>> return 0; > > > >>> > > > >>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > > >>> state); > > > >>> } > > > >>> > > > >>> -static int intel_plane_atomic_check(struct drm_plane *plane, > > > >>> - struct drm_plane_state *new_plane_state) > > > >>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > > > >>> + struct drm_plane_state *new_drm_plane_state) > > > >>> { > > > >>> - struct drm_atomic_state *state = new_plane_state->state; > > > >>> - const struct drm_plane_state *old_plane_state = > > > >>> - drm_atomic_get_old_plane_state(state, plane); > > > >>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > > > >>> - const struct drm_crtc_state *old_crtc_state; > > > >>> - struct drm_crtc_state *new_crtc_state; > > > >>> - > > > >>> - new_plane_state->visible = false; > > > >>> + struct intel_atomic_state *state = > > > >>> + to_intel_atomic_state(new_drm_plane_state->state); > > > >>> + struct intel_plane *plane = to_intel_plane(drm_plane); > > > >>> + const struct intel_plane_state *old_plane_state = > > > >>> + intel_atomic_get_old_plane_state(state, plane); > > > >>> + struct intel_plane_state *new_plane_state = > > > >>> + to_intel_plane_state(new_drm_plane_state); > > > >>> + struct intel_crtc *crtc = to_intel_crtc( > > > >>> + new_plane_state->base.crtc ?: > > > >>> + old_plane_state->base.crtc); > > > >>> + const struct intel_crtc_state *old_crtc_state; > > > >>> + struct intel_crtc_state *new_crtc_state; > > > >>> + struct intel_plane *linked = old_plane_state->linked_plane; > > > >>> + int ret; > > > >>> + const struct intel_plane_state *old_linked_state; > > > >>> + struct intel_plane_state *new_linked_state = NULL; > > > >>> + > > > >>> + if (linked) { > > > >>> + /* > > > >>> + * Make sure a previously linked plane (and implicitly, the CRTC) > > > >>> + * is part of the atomic commit. > > > >>> + */ > > > >>> + if (!intel_atomic_get_new_plane_state(state, linked)) { > > > >>> + new_linked_state = intel_atomic_get_plane_state(state, linked); > > > >>> + if (IS_ERR(new_linked_state)) > > > >>> + return PTR_ERR(new_linked_state); > > > >>> + } > > > >>> + > > > >>> + old_linked_state = > > > >>> + intel_atomic_get_old_plane_state(state, linked); > > > >>> + > > > >>> + /* > > > >>> + * This will happen when we're the Y plane. In which case > > > >>> + * old/new_state->crtc are both NULL. We still need to perform > > > >>> + * updates on the linked plane. > > > >>> + */ > > > >>> + if (!crtc) > > > >>> + crtc = to_intel_crtc(old_linked_state->base.crtc); > > > >>> + > > > >>> + WARN_ON(!crtc); > > > >>> + } > > > >>> + > > > >>> + new_plane_state->base.visible = false; > > > >>> if (!crtc) > > > >>> return 0; > > > >>> > > > >>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > > > >>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > >>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > >>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > > >>> + > > > >>> + if (new_linked_state && > > > >>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > > > >>> + /* > > > >>> + * This function is called from drm_atomic_helper_check_planes(), which > > > >>> + * will normally check the newly added plane for us, but since we're > > > >>> + * already in that function, it won't check the plane if our index > > > >>> + * is bigger than the linked index because of the > > > >>> + * for_each_oldnew_plane_in_state() call. > > > >>> + */ > > > >>> + new_crtc_state->base.planes_changed = true; > > > >>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > > > >>> + old_linked_state, new_linked_state); > > > >>> + if (ret) > > > >>> + return ret; > > > >>> + } > > > >> This is all rather confusing. Can't we just do a preprocessing step > > > >> before check_planes() to add the linked planes as needed, and then > > > >> let the normal check_planes() do its thing? > > > > Also one thing that slightly worries me is what happens if someone adds > > > > more planes to the state after this has all been done. I guess > > > > currently those cases would either come from the tail end of > > > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > > > > would appear that wm/ddb is the only piece of code that can do this > > > > sort of thing. > > > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). > > > Otherwise you can't rerun any validation as required. > > > > You shouldn't need validation for eg. dpms on/off. I guess we currently > > do that although we shouldn't have to. > > > > > > > > I've now added a function icl_add_linked_planes helper that iterates over all planes in > > > the state, and adds any linked planes to the transaction. > > > > > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked > > > planes are added, without doing it from intel_plane_atomic_check() > > > > > > WM will continue to do its own thing, since it's a design error IMO that it even adds > > > planes to the state to begin with. :) > > > > It pretty much has to. The design error we have at the moment is not > > programming the watermarks from the update_plane()/disable_plane(). > > That one I've attempted to fix in: > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > > > And supposedly that one does fix bugs related to watermarks vs. > > plane updates. > > Where did the bugs arise? Were we unsuccessful at actually evading the > vblank (leading to the planes and watermarks taking effect on different > vblanks) or is it something else? There are a few issues here: - write to any plane registers apart from SURF will cancel an already pending plane update. Well, it's not 100% cancelled as some registers aren't part of the SURF based arming mechanism but IIRC they still cause a cancellation. This means doing a watermark update before a pending plane update was latched cancels most of the plane update. This at least caused the cursor to remain on in when it should have been turned off. - overlapping DDB allocations can hard hang the box. So any vblank that sneaks in while we've partiially reprogrammed the ddb could be a death sentence. A suggested solution was to turn off the planes before ddb reprogramming but that didn't work out so well when I tried it due to the whole cancellation thing. So I pulled in the ddb/wm programming into the normal plane update. That means no more accidental cancellations from elsewhere. And to avoid any ddb overlaps we simply sequence the plane updates carefully. It's pretty much the same algorithm that we use to avoid ddb overlaps between pipes, with the exception that we don't need the vblank waits. So if a vblank does sneak at any point during the sequence we can be assured that the partially latched state does not have any overlapping ddb allocations.
On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote: > On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote: > > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote: > > > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote: > > > > Op 21-09-18 om 21:31 schreef Ville Syrjälä: > > > > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote: > > > > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote: > > > > >>> To make NV12 working on icl, we need to update 2 planes simultaneously. > > > > >>> I've chosen to do this in the CRTC step after plane validation is done, > > > > >>> so we know what planes are (in)visible. The linked Y plane will get > > > > >>> updated in intel_plane_update_planes_on_crtc(), by the call to > > > > >>> update_slave, which gets the master's plane_state as argument. > > > > >>> > > > > >>> The link requires both planes for atomic_update to work, > > > > >>> so make sure skl_ddb_add_affected_planes() adds both states. > > > > >>> > > > > >>> Changes since v1: > > > > >>> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers. > > > > >>> - Put all the state updating login in intel_plane_atomic_check_with_state(). > > > > >>> - Clean up changes in intel_plane_atomic_check(). > > > > >>> Changes since v2: > > > > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state. > > > > >>> - Move visibility changes to preparation patch. > > > > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane. > > > > >>> > > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > >>> > > > > >>> fixup Y/UV Linkage > > > > >>> > > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > >>> --- > > > > >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++---- > > > > >>> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++ > > > > >>> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++ > > > > >>> drivers/gpu/drm/i915/intel_pm.c | 12 ++- > > > > >>> 4 files changed, 210 insertions(+), 18 deletions(-) > > > > >>> > > > > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > > > > >>> index 984bc1f26625..522699085a59 100644 > > > > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > > > > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > > > > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > > > >>> crtc_state->nv12_planes &= ~BIT(intel_plane->id); > > > > >>> intel_state->base.visible = false; > > > > >>> > > > > >>> - /* If this is a cursor plane, no further checks are needed. */ > > > > >>> + /* Destroy the link */ > > > > >>> + intel_state->linked_plane = NULL; > > > > >>> + intel_state->slave = false; > > > > >>> + > > > > >>> + /* If this is a cursor or Y plane, no further checks are needed. */ > > > > >>> if (!intel_state->base.crtc && !old_plane_state->base.crtc) > > > > >>> return 0; > > > > >>> > > > > >>> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > > > > >>> state); > > > > >>> } > > > > >>> > > > > >>> -static int intel_plane_atomic_check(struct drm_plane *plane, > > > > >>> - struct drm_plane_state *new_plane_state) > > > > >>> +static int intel_plane_atomic_check(struct drm_plane *drm_plane, > > > > >>> + struct drm_plane_state *new_drm_plane_state) > > > > >>> { > > > > >>> - struct drm_atomic_state *state = new_plane_state->state; > > > > >>> - const struct drm_plane_state *old_plane_state = > > > > >>> - drm_atomic_get_old_plane_state(state, plane); > > > > >>> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; > > > > >>> - const struct drm_crtc_state *old_crtc_state; > > > > >>> - struct drm_crtc_state *new_crtc_state; > > > > >>> - > > > > >>> - new_plane_state->visible = false; > > > > >>> + struct intel_atomic_state *state = > > > > >>> + to_intel_atomic_state(new_drm_plane_state->state); > > > > >>> + struct intel_plane *plane = to_intel_plane(drm_plane); > > > > >>> + const struct intel_plane_state *old_plane_state = > > > > >>> + intel_atomic_get_old_plane_state(state, plane); > > > > >>> + struct intel_plane_state *new_plane_state = > > > > >>> + to_intel_plane_state(new_drm_plane_state); > > > > >>> + struct intel_crtc *crtc = to_intel_crtc( > > > > >>> + new_plane_state->base.crtc ?: > > > > >>> + old_plane_state->base.crtc); > > > > >>> + const struct intel_crtc_state *old_crtc_state; > > > > >>> + struct intel_crtc_state *new_crtc_state; > > > > >>> + struct intel_plane *linked = old_plane_state->linked_plane; > > > > >>> + int ret; > > > > >>> + const struct intel_plane_state *old_linked_state; > > > > >>> + struct intel_plane_state *new_linked_state = NULL; > > > > >>> + > > > > >>> + if (linked) { > > > > >>> + /* > > > > >>> + * Make sure a previously linked plane (and implicitly, the CRTC) > > > > >>> + * is part of the atomic commit. > > > > >>> + */ > > > > >>> + if (!intel_atomic_get_new_plane_state(state, linked)) { > > > > >>> + new_linked_state = intel_atomic_get_plane_state(state, linked); > > > > >>> + if (IS_ERR(new_linked_state)) > > > > >>> + return PTR_ERR(new_linked_state); > > > > >>> + } > > > > >>> + > > > > >>> + old_linked_state = > > > > >>> + intel_atomic_get_old_plane_state(state, linked); > > > > >>> + > > > > >>> + /* > > > > >>> + * This will happen when we're the Y plane. In which case > > > > >>> + * old/new_state->crtc are both NULL. We still need to perform > > > > >>> + * updates on the linked plane. > > > > >>> + */ > > > > >>> + if (!crtc) > > > > >>> + crtc = to_intel_crtc(old_linked_state->base.crtc); > > > > >>> + > > > > >>> + WARN_ON(!crtc); > > > > >>> + } > > > > >>> + > > > > >>> + new_plane_state->base.visible = false; > > > > >>> if (!crtc) > > > > >>> return 0; > > > > >>> > > > > >>> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > > > > >>> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > > >>> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > > >>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > > > >>> + > > > > >>> + if (new_linked_state && > > > > >>> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { > > > > >>> + /* > > > > >>> + * This function is called from drm_atomic_helper_check_planes(), which > > > > >>> + * will normally check the newly added plane for us, but since we're > > > > >>> + * already in that function, it won't check the plane if our index > > > > >>> + * is bigger than the linked index because of the > > > > >>> + * for_each_oldnew_plane_in_state() call. > > > > >>> + */ > > > > >>> + new_crtc_state->base.planes_changed = true; > > > > >>> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, > > > > >>> + old_linked_state, new_linked_state); > > > > >>> + if (ret) > > > > >>> + return ret; > > > > >>> + } > > > > >> This is all rather confusing. Can't we just do a preprocessing step > > > > >> before check_planes() to add the linked planes as needed, and then > > > > >> let the normal check_planes() do its thing? > > > > > Also one thing that slightly worries me is what happens if someone adds > > > > > more planes to the state after this has all been done. I guess > > > > > currently those cases would either come from the tail end of > > > > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it > > > > > would appear that wm/ddb is the only piece of code that can do this > > > > > sort of thing. > > > > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes(). > > > > Otherwise you can't rerun any validation as required. > > > > > > You shouldn't need validation for eg. dpms on/off. I guess we currently > > > do that although we shouldn't have to. > > > > > > > > > > > I've now added a function icl_add_linked_planes helper that iterates over all planes in > > > > the state, and adds any linked planes to the transaction. > > > > > > > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked > > > > planes are added, without doing it from intel_plane_atomic_check() > > > > > > > > WM will continue to do its own thing, since it's a design error IMO that it even adds > > > > planes to the state to begin with. :) > > > > > > It pretty much has to. The design error we have at the moment is not > > > programming the watermarks from the update_plane()/disable_plane(). > > > That one I've attempted to fix in: > > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > > > > > And supposedly that one does fix bugs related to watermarks vs. > > > plane updates. > > > > Where did the bugs arise? Were we unsuccessful at actually evading the > > vblank (leading to the planes and watermarks taking effect on different > > vblanks) or is it something else? > > There are a few issues here: > - write to any plane registers apart from SURF will cancel an already > pending plane update. Well, it's not 100% cancelled as some registers > aren't part of the SURF based arming mechanism but IIRC they still > cause a cancellation. This means doing a watermark update before a > pending plane update was latched cancels most of the plane update. > This at least caused the cursor to remain on in when it should have > been turned off. Wow, I think I've run into this problem before, but never figured out what the exact root cause was. I tried to write an IGT to capture it a while back, but the behavior went away with my simpler tests, probably because I wasn't changing enough settings to trigger the necessary watermark updates. The behavior was puzzling since some of the plane updates actually would take effect (e.g., PLANE_POS being zeroed would move the plane back to 0,0), but others wouldn't (most notable the enable/disable bit in PLANE_CTL). So the result might be a garbage rectangle in the upper left corner of the screen or a storm of GTT page faults that would bring make the system unusable. Is this actually expected behavior that's documented somewhere in the bspec, or is it just something you've discovered through experimentation? I couldn't find any explanation for updates getting partially unarmed when I went through the bspec a while back, but I may have overlooked something. > - overlapping DDB allocations can hard hang the box. So any vblank that > sneaks in while we've partiially reprogrammed the ddb could be a > death sentence. A suggested solution was to turn off the planes > before ddb reprogramming but that didn't work out so well when I > tried it due to the whole cancellation thing. > > So I pulled in the ddb/wm programming into the normal plane update. > That means no more accidental cancellations from elsewhere. > > And to avoid any ddb overlaps we simply sequence the plane updates > carefully. It's pretty much the same algorithm that we use to avoid > ddb overlaps between pipes, with the exception that we don't need the > vblank waits. So if a vblank does sneak at any point during the > sequence we can be assured that the partially latched state does not > have any overlapping ddb allocations. Sounds reasonable. Do you think we should try to land your work before Maarten's gen11 NV12 patches? Matt > > -- > Ville Syrjälä > Intel
On Tue, Sep 25, 2018 at 01:18:43PM -0700, Matt Roper wrote: > On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote: > > > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote: <snip> > > > > It pretty much has to. The design error we have at the moment is not > > > > programming the watermarks from the update_plane()/disable_plane(). > > > > That one I've attempted to fix in: > > > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence > > > > > > > > And supposedly that one does fix bugs related to watermarks vs. > > > > plane updates. > > > > > > Where did the bugs arise? Were we unsuccessful at actually evading the > > > vblank (leading to the planes and watermarks taking effect on different > > > vblanks) or is it something else? > > > > There are a few issues here: > > - write to any plane registers apart from SURF will cancel an already > > pending plane update. Well, it's not 100% cancelled as some registers > > aren't part of the SURF based arming mechanism but IIRC they still > > cause a cancellation. This means doing a watermark update before a > > pending plane update was latched cancels most of the plane update. > > This at least caused the cursor to remain on in when it should have > > been turned off. > > Wow, I think I've run into this problem before, but never figured out > what the exact root cause was. I tried to write an IGT to capture it a > while back, but the behavior went away with my simpler tests, probably > because I wasn't changing enough settings to trigger the necessary > watermark updates. > > The behavior was puzzling since some of the plane updates actually would > take effect (e.g., PLANE_POS being zeroed would move the plane back to > 0,0), but others wouldn't (most notable the enable/disable bit in > PLANE_CTL). So the result might be a garbage rectangle in the upper > left corner of the screen or a storm of GTT page faults that would bring > make the system unusable. > > Is this actually expected behavior that's documented somewhere in the > bspec, or is it just something you've discovered through > experimentation? I couldn't find any explanation for updates getting > partially unarmed when I went through the bspec a while back, but I may > have overlooked something. I don't see the cancellation behaviour mentioned in the current spec. It was actually documented for gen3 cursors (see eg. [1]) but apart from that there is no mention of it in any spec AFAICS. I believe only the cursors had this behaviour on gen3, and I think even that was changed again to not cancel around the gen4-5 timeframe. SKL seems to have re-introduced it for all planes for whatever reason. But I've never performed an exhaustive test on all the platforms/planes to confirm which way each one works. [1] commit d34cfebbf9cc ("drm/i915: Fix cursor updates on some platforms") > > > - overlapping DDB allocations can hard hang the box. So any vblank that > > sneaks in while we've partiially reprogrammed the ddb could be a > > death sentence. A suggested solution was to turn off the planes > > before ddb reprogramming but that didn't work out so well when I > > tried it due to the whole cancellation thing. > > > > So I pulled in the ddb/wm programming into the normal plane update. > > That means no more accidental cancellations from elsewhere. > > > > And to avoid any ddb overlaps we simply sequence the plane updates > > carefully. It's pretty much the same algorithm that we use to avoid > > ddb overlaps between pipes, with the exception that we don't need the > > vblank waits. So if a vblank does sneak at any point during the > > sequence we can be assured that the partially latched state does not > > have any overlapping ddb allocations. > > Sounds reasonable. Do you think we should try to land your work before > Maarten's gen11 NV12 patches? I wouldn't expect significant rebase hurdles from the nv12 work. So I'm ok with landing the nv12 stuff first. I'm not 100% sure I managed to handle the cursor ddb correctly in that branch. We always allocate a ddb chunk for the cursor even if it's disabled so it behaves slightly differently compared to the other planes when it comes to updating the ddb from .disable_plane(). In fact I don't even remember anymore how I handled that case. So there might still be some work left to polish that branch. I'm a bit busy with other things atm, so it'll probably be a while. Unless someone else wants to pick up where I left off, which is totally fine by me.
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index 984bc1f26625..522699085a59 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ crtc_state->nv12_planes &= ~BIT(intel_plane->id); intel_state->base.visible = false; - /* If this is a cursor plane, no further checks are needed. */ + /* Destroy the link */ + intel_state->linked_plane = NULL; + intel_state->slave = false; + + /* If this is a cursor or Y plane, no further checks are needed. */ if (!intel_state->base.crtc && !old_plane_state->base.crtc) return 0; @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ state); } -static int intel_plane_atomic_check(struct drm_plane *plane, - struct drm_plane_state *new_plane_state) +static int intel_plane_atomic_check(struct drm_plane *drm_plane, + struct drm_plane_state *new_drm_plane_state) { - struct drm_atomic_state *state = new_plane_state->state; - const struct drm_plane_state *old_plane_state = - drm_atomic_get_old_plane_state(state, plane); - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; - const struct drm_crtc_state *old_crtc_state; - struct drm_crtc_state *new_crtc_state; - - new_plane_state->visible = false; + struct intel_atomic_state *state = + to_intel_atomic_state(new_drm_plane_state->state); + struct intel_plane *plane = to_intel_plane(drm_plane); + const struct intel_plane_state *old_plane_state = + intel_atomic_get_old_plane_state(state, plane); + struct intel_plane_state *new_plane_state = + to_intel_plane_state(new_drm_plane_state); + struct intel_crtc *crtc = to_intel_crtc( + new_plane_state->base.crtc ?: + old_plane_state->base.crtc); + const struct intel_crtc_state *old_crtc_state; + struct intel_crtc_state *new_crtc_state; + struct intel_plane *linked = old_plane_state->linked_plane; + int ret; + const struct intel_plane_state *old_linked_state; + struct intel_plane_state *new_linked_state = NULL; + + if (linked) { + /* + * Make sure a previously linked plane (and implicitly, the CRTC) + * is part of the atomic commit. + */ + if (!intel_atomic_get_new_plane_state(state, linked)) { + new_linked_state = intel_atomic_get_plane_state(state, linked); + if (IS_ERR(new_linked_state)) + return PTR_ERR(new_linked_state); + } + + old_linked_state = + intel_atomic_get_old_plane_state(state, linked); + + /* + * This will happen when we're the Y plane. In which case + * old/new_state->crtc are both NULL. We still need to perform + * updates on the linked plane. + */ + if (!crtc) + crtc = to_intel_crtc(old_linked_state->base.crtc); + + WARN_ON(!crtc); + } + + new_plane_state->base.visible = false; if (!crtc) return 0; - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + + if (new_linked_state && + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) { + /* + * This function is called from drm_atomic_helper_check_planes(), which + * will normally check the newly added plane for us, but since we're + * already in that function, it won't check the plane if our index + * is bigger than the linked index because of the + * for_each_oldnew_plane_in_state() call. + */ + new_crtc_state->base.planes_changed = true; + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, + old_linked_state, new_linked_state); + if (ret) + return ret; + } - return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state), - to_intel_crtc_state(new_crtc_state), - to_intel_plane_state(old_plane_state), - to_intel_plane_state(new_plane_state)); + return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state, + old_plane_state, new_plane_state); } void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, @@ -187,6 +240,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state, trace_intel_update_plane(&plane->base, crtc); plane->update_plane(plane, new_crtc_state, new_plane_state); + } else if (new_plane_state->slave) { + struct intel_plane *master = + new_plane_state->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(old_state, master); + + trace_intel_update_plane(&plane->base, crtc); + + plane->update_slave(plane, new_crtc_state, new_plane_state); } else { trace_intel_disable_plane(&plane->base, crtc); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 390907d77ecd..19cd6bbb43c4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10726,6 +10726,60 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state, return true; } +static int icl_check_nv12_planes(struct drm_i915_private *dev_priv, + struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state) +{ + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state); + struct intel_plane *plane, *aux; + + if (INTEL_GEN(dev_priv) < 11 || !crtc_state->nv12_planes) + return 0; + + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) { + struct intel_plane_state *plane_state, *aux_state; + struct drm_plane_state *drm_aux_state = NULL; + + if (!(crtc_state->nv12_planes & BIT(plane->id))) + continue; + + plane_state = intel_atomic_get_new_plane_state(state, plane); + if (!plane_state) + continue; + + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) { + if (!icl_is_nv12_y_plane(aux->id)) + continue; + + if (crtc_state->active_planes & BIT(aux->id)) + continue; + + drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base); + if (IS_ERR(drm_aux_state)) + return PTR_ERR(drm_aux_state); + + break; + } + + if (!drm_aux_state) { + DRM_DEBUG_KMS("Need %d free Y planes for NV12\n", + hweight8(crtc_state->nv12_planes)); + + return -EINVAL; + } + + plane_state->linked_plane = aux; + + aux_state = to_intel_plane_state(drm_aux_state); + aux_state->slave = true; + aux_state->linked_plane = plane; + crtc_state->active_planes |= BIT(aux->id); + DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name); + } + + return 0; +} + static int intel_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) { @@ -10797,6 +10851,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, if (mode_changed) ret = skl_update_scaler_crtc(pipe_config); + if (!ret) + ret = icl_check_nv12_planes(dev_priv, intel_crtc, + pipe_config); if (!ret) ret = skl_check_pipe_max_pixel_rate(intel_crtc, pipe_config); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8073a85d7178..29c7a4bb484d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -539,6 +539,26 @@ struct intel_plane_state { */ int scaler_id; + /* + * linked_plane: + * + * ICL planar formats require 2 planes that are updated as pairs. + * This member is used to make sure the other plane is also updated + * when required, and for update_slave() to find the correct + * plane_state to pass as argument. + */ + struct intel_plane *linked_plane; + + /* + * slave: + * If set don't update use the linked plane's state for updating + * this plane during atomic commit with the update_slave() callback. + * + * It's also used by the watermark code to ignore wm calculations on + * this plane. They're calculated by the linked plane's wm code. + */ + bool slave; + struct drm_intel_sprite_colorkey ckey; }; @@ -973,6 +993,9 @@ 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, struct intel_crtc *crtc); bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe); @@ -1330,6 +1353,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) return container_of(intel_hdmi, struct intel_digital_port, hdmi); } +static inline struct intel_plane_state * +intel_atomic_get_plane_state(struct intel_atomic_state *state, + struct intel_plane *plane) +{ + struct drm_plane_state *ret = + drm_atomic_get_plane_state(&state->base, &plane->base); + + if (IS_ERR(ret)) + return ERR_CAST(ret); + + return to_intel_plane_state(ret); +} + +static inline struct intel_plane_state * +intel_atomic_get_old_plane_state(struct intel_atomic_state *state, + struct intel_plane *plane) +{ + return to_intel_plane_state(drm_atomic_get_old_plane_state(&state->base, + &plane->base)); +} + static inline struct intel_plane_state * intel_atomic_get_new_plane_state(struct intel_atomic_state *state, struct intel_plane *plane) @@ -2143,6 +2187,15 @@ int skl_plane_check(struct intel_crtc_state *crtc_state, int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state); int chv_plane_check_rotation(const struct intel_plane_state *plane_state); +static inline bool icl_is_nv12_y_plane(enum plane_id id) +{ + /* Don't need to do a gen check, these planes are only available on gen11 */ + if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5) + return true; + + return false; +} + /* intel_tv.c */ void intel_tv_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1db9b8328275..d76d93452137 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5137,11 +5137,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb; - struct drm_plane_state *plane_state; struct drm_plane *plane; enum pipe pipe = intel_crtc->pipe; drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { + struct drm_plane_state *plane_state; + struct intel_plane *linked; enum plane_id plane_id = to_intel_plane(plane)->id; if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id], @@ -5153,6 +5154,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) return PTR_ERR(plane_state); + + /* Make sure linked plane is updated too */ + linked = to_intel_plane_state(plane_state)->linked_plane; + if (!linked) + continue; + + plane_state = drm_atomic_get_plane_state(state, &linked->base); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); } return 0;