From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 15/23] drm/i915: Link planes in a bigjoiner configuration.
Date: Tue, 1 Oct 2019 20:21:06 +0300 [thread overview]
Message-ID: <20191001172106.GR1208@intel.com> (raw)
In-Reply-To: <20191001164409.GJ1869@mdroper-desk1.amr.corp.intel.com>
On Tue, Oct 01, 2019 at 09:44:09AM -0700, Matt Roper wrote:
> On Fri, Sep 20, 2019 at 01:42:27PM +0200, Maarten Lankhorst wrote:
> > Make sure that when a plane is set in a bigjoiner mode, we will add
> > their counterpart to the atomic state as well. This will allow us to
> > make sure all state is available when planes are checked.
> >
> > Because of the funny interactions with bigjoiner and planar YUV
> > formats, we may end up adding a lot of planes, so we have to keep
> > iterating until we no longer add any planes.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > .../gpu/drm/i915/display/intel_atomic_plane.c | 31 +++-
> > .../gpu/drm/i915/display/intel_atomic_plane.h | 4 +
> > drivers/gpu/drm/i915/display/intel_display.c | 142 ++++++++++++++++--
> > .../drm/i915/display/intel_display_types.h | 11 ++
> > 4 files changed, 172 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 964db7774d10..cc088676f0a2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -182,16 +182,36 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > old_plane_state, new_plane_state);
> > }
> >
> > -static struct intel_crtc *
> > -get_crtc_from_states(const struct intel_plane_state *old_plane_state,
> > - const struct intel_plane_state *new_plane_state)
> > -{
> > +struct intel_crtc *
> > +intel_plane_get_crtc_from_states(struct intel_atomic_state *state,
>
> This function name seems ambiguous now since it isn't clear whether
> we're getting the plane's hardware CRTC or its uapi CRTC. Maybe call it
> something like intel_plane_state_get_uapi_crtc()?
I was actually wondering if we shouldn't just do
get_crtc(plane->pipe) here. We still don't expose any planes
that can switch pipes so feels like this stuff is unecessary
complexity we don't particularly need.
>
>
>
> > + const struct intel_plane_state *old_plane_state,
> > + const struct intel_plane_state *new_plane_state)
> > + {
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + struct intel_plane *plane = to_intel_plane(new_plane_state->base.plane);
> > +
> > if (new_plane_state->base.crtc)
> > return to_intel_crtc(new_plane_state->base.crtc);
> >
> > if (old_plane_state->base.crtc)
> > return to_intel_crtc(old_plane_state->base.crtc);
> >
> > + if (new_plane_state->bigjoiner_slave) {
> > + const struct intel_plane_state *new_master_plane_state =
> > + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane);
> > +
> > + if (new_master_plane_state->base.crtc)
> > + return intel_get_crtc_for_pipe(dev_priv, plane->pipe);
> > + }
>
> Will this cause problems if we adjust properties of planes on a a
> uapi-disabled CRTC? E.g., I believe userspace can fiddle with stuff
> like rotation properties on disabled planes/crtcs and now I believe that
> causes us to needlessly pull in the bigjoiner master crtc and
> corresponding plane?
Not sure it's worth the effort to really try to avoid such things. If
userspace keeps fiddling with disabled stuff all the time it feels a
bit broken to me.
On a semi-related note I've occasionally pondered about trying to
filter out nop commits which don't send out events, even on active
planes/crtcs. But I guess that's already starting to be a change to
the semantics so probably not something that could be done without
a new flag/etc.
>
> > +
> > + if (old_plane_state->bigjoiner_slave) {
> > + const struct intel_plane_state *old_master_plane_state =
> > + intel_atomic_get_old_plane_state(state, old_plane_state->bigjoiner_plane);
> > +
> > + if (old_master_plane_state->base.crtc)
> > + return intel_get_crtc_for_pipe(dev_priv, plane->pipe);
> > + }
> > +
> > return NULL;
> > }
> >
> > @@ -206,7 +226,8 @@ static int intel_plane_atomic_check(struct drm_plane *_plane,
> > const struct intel_plane_state *old_plane_state =
> > intel_atomic_get_old_plane_state(state, plane);
> > struct intel_crtc *crtc =
> > - get_crtc_from_states(old_plane_state, new_plane_state);
> > + intel_plane_get_crtc_from_states(state, old_plane_state,
> > + new_plane_state);
> > const struct intel_crtc_state *old_crtc_state;
> > struct intel_crtc_state *new_crtc_state;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > index 33fb85cd3909..901a50e6e2d3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > @@ -42,5 +42,9 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> > struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *old_plane_state,
> > struct intel_plane_state *plane_state);
> > +struct intel_crtc *
> > +intel_plane_get_crtc_from_states(struct intel_atomic_state *state,
> > + const struct intel_plane_state *old_plane_state,
> > + const struct intel_plane_state *new_plane_state);
> >
> > #endif /* __INTEL_ATOMIC_PLANE_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index df588bf47559..06ceac4f1436 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -11811,24 +11811,101 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
> > return true;
> > }
> >
> > +static int icl_add_dependent_planes(struct intel_atomic_state *state,
> > + struct intel_plane_state *plane_state)
> > +{
> > + struct intel_plane_state *new_plane_state;
> > + struct intel_plane *plane;
> > + int ret = 0;
> > +
> > + plane = plane_state->bigjoiner_plane;
> > + if (plane && !intel_atomic_get_new_plane_state(state, plane)) {
> > + new_plane_state = intel_atomic_get_plane_state(state, plane);
> > + if (IS_ERR(new_plane_state))
> > + return PTR_ERR(new_plane_state);
> > +
> > + ret = 1;
> > + }
> > +
> > + plane = plane_state->planar_linked_plane;
> > + if (plane && !intel_atomic_get_new_plane_state(state, plane)) {
> > + new_plane_state = intel_atomic_get_plane_state(state, plane);
> > + if (IS_ERR(new_plane_state))
> > + return PTR_ERR(new_plane_state);
> > +
> > + ret = 1;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int icl_add_linked_planes(struct intel_atomic_state *state)
> > {
> > - struct intel_plane *plane, *linked;
> > - struct intel_plane_state *plane_state, *linked_plane_state;
> > + struct intel_plane *plane;
> > + struct intel_plane_state *old_plane_state, *new_plane_state;
> > + struct intel_crtc *crtc, *linked_crtc;
> > + struct intel_crtc_state *old_crtc_state, *new_crtc_state, *linked_crtc_state;
> > + bool added;
> > int i;
> >
> > - for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> > - linked = plane_state->planar_linked_plane;
> > + /*
> > + * Iteratively add plane_state->linked_plane and plane_state->bigjoiner_plane
> > + *
> > + * This needs to be done repeatedly, because of is a funny interaction;
> > + * the Y-plane may be assigned differently on the other bigjoiner crtc,
> > + * and we could end up with the following evil recursion, when only adding a
> > + * single plane to state:
> > + *
> > + * XRGB8888 master plane 6 adds NV12 slave Y-plane 6, which adds slave UV plane 0,
> > + * which adds master UV plane 0, which adds master Y-plane 7, which adds XRGB8888
> > + * slave plane 7.
> > + *
> > + * We could pull in even more because of old_plane_state vs new_plane_state.
> > + *
> > + * Max depth = 5 (or 7 for evil case) in this case.
> > + * Number of passes will be less, because newly added planes show up in the
> > + * same iteration round when added_plane->index > plane->index.
> > + */
> > + do {
> > + added = false;
> >
> > - if (!linked)
> > - continue;
> > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> > + int ret, ret2;
> > +
> > + ret = icl_add_dependent_planes(state, old_plane_state);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret2 = icl_add_dependent_planes(state, new_plane_state);
> > + if (ret2 < 0)
> > + return ret2;
> >
> > - linked_plane_state = intel_atomic_get_plane_state(state, linked);
> > - if (IS_ERR(linked_plane_state))
> > - return PTR_ERR(linked_plane_state);
> > + added |= ret || ret2;
I suspect passing &added to the functions would be less annoying
than this magic return 1 stuff.
> > + }
> > + } while (added);
> > +
> > + /*
> > + * Make sure bigjoiner slave crtc's are also pulled in. This is not done automatically
> > + * when adding slave planes, because plane_state->crtc is null.
> > + */
> > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > + linked_crtc = old_crtc_state->bigjoiner_linked_crtc;
> > + if (linked_crtc) {
> > + linked_crtc_state =
> > + intel_atomic_get_crtc_state(&state->base, linked_crtc);
> > +
> > + if (IS_ERR(linked_crtc_state))
> > + return PTR_ERR(linked_crtc_state);
> > + }
> > +
> > + linked_crtc = new_crtc_state->bigjoiner_linked_crtc;
> > + if (linked_crtc && linked_crtc != old_crtc_state->bigjoiner_linked_crtc) {
> > + linked_crtc_state =
> > + intel_atomic_get_crtc_state(&state->base, linked_crtc);
> >
> > - WARN_ON(linked_plane_state->planar_linked_plane != plane);
> > - WARN_ON(linked_plane_state->planar_slave == plane_state->planar_slave);
> > + if (IS_ERR(linked_crtc_state))
> > + return PTR_ERR(linked_crtc_state);
> > + }
> > }
> >
> > return 0;
> > @@ -13799,6 +13876,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state)
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > struct intel_crtc_state *old_crtc_state, *new_crtc_state, *slave_crtc_state, *master_crtc_state;
> > struct intel_crtc *crtc, *slave, *master;
> > + struct intel_plane *plane;
> > int i, ret = 0;
> >
> > if (INTEL_GEN(dev_priv) < 11)
> > @@ -13894,6 +13972,48 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state)
> > return ret;
> > }
> >
> > + /*
> > + * Setup and teardown the new bigjoiner plane mappings.
> > + */
> > + for_each_intel_plane(&dev_priv->drm, plane) {
> > + struct intel_plane_state *plane_state;
> > + struct intel_plane *other_plane = NULL;
> > +
> > + crtc = intel_get_crtc_for_pipe(dev_priv, plane->pipe);
> > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +
> > + if (!new_crtc_state || !needs_modeset(new_crtc_state))
> > + continue;
> > +
> > + if (new_crtc_state->bigjoiner) {
> > + struct intel_crtc *other_crtc =
> > + new_crtc_state->bigjoiner_linked_crtc;
> > + bool found = false;
> > +
> > + for_each_intel_plane_on_crtc(&dev_priv->drm, other_crtc, other_plane) {
> > + if (other_plane->id != plane->id)
> > + continue;
> > +
> > + found = true;
> > + break;
> > + }
> > +
> > + /* All pipes should have identical planes. */
> > + if (WARN_ON(!found))
> > + return -EINVAL;
> > + } else if (!old_crtc_state->bigjoiner) {
> > + continue;
> > + }
> > +
> > + plane_state = intel_atomic_get_plane_state(state, plane);
> > + if (IS_ERR(plane_state))
> > + return PTR_ERR(plane_state);
> > +
> > + plane_state->bigjoiner_plane = other_plane;
> > + plane_state->bigjoiner_slave = new_crtc_state->bigjoiner_slave;
> > + }
>
> Is my understanding correct that we always grab the corresponding plane
> on the other CRTC when the big joiner is active? I.e., even in cases
> where the uapi plane is restricted to just one half of the screen, we're
> still going to grab the bigjoiner slave plane? I guess trying to
> restrict this to only cases where the uapi plane covers both pipes would
> make this whole thing even more complicated.
Yeah, I'm thinking we can just ignore that particular optimization.
Would need to somehow convince ourselves early that the plane still
remains on its own half of the screen, or we'd need to come up with some
kind of retry loop for this that can restart if a new plane gets added
in the middle of the process. IMO not worth the hassle.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-10-01 17:21 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-20 11:42 [PATCH 01/23] drm/i915/dp: Fix dsc bpp calculations, v2 Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 02/23] HAX drm/i915: Disable FEC entirely for now Maarten Lankhorst
2019-09-23 13:08 ` Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 03/23] drm/i915: Prepare to split crtc state in uapi and hw state Maarten Lankhorst
2019-09-24 23:40 ` Matt Roper
2019-09-25 9:09 ` Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 04/23] drm/i915: Handle a few more cases for hw/sw split Maarten Lankhorst
2019-09-24 23:40 ` Matt Roper
2019-09-20 11:42 ` [PATCH 05/23] drm/i915: Complete sw/hw split Maarten Lankhorst
2019-09-24 23:41 ` Matt Roper
2019-09-25 9:29 ` Maarten Lankhorst
2019-09-25 13:01 ` Ville Syrjälä
2019-09-25 14:12 ` Maarten Lankhorst
2019-09-25 14:18 ` Maarten Lankhorst
2019-09-25 14:54 ` Ville Syrjälä
2019-09-20 11:42 ` [PATCH 06/23] drm/i915: Get rid of crtc_state->fb_changed Maarten Lankhorst
2019-09-24 23:44 ` Matt Roper
2019-09-20 11:42 ` [PATCH 07/23] drm/i915: Remove begin/finish_crtc_commit Maarten Lankhorst
2019-09-25 4:17 ` Matt Roper
2019-09-25 14:14 ` Maarten Lankhorst
2019-09-25 22:14 ` Manasi Navare
2019-09-20 11:42 ` [PATCH 08/23] drm/i915: Rename planar linked plane variables Maarten Lankhorst
2019-09-25 4:30 ` Matt Roper
2019-09-20 11:42 ` [PATCH 09/23] drm/i915: Do not add all planes when checking scalers on glk+ Maarten Lankhorst
2019-09-25 4:55 ` Matt Roper
2019-09-25 12:45 ` Maarten Lankhorst
2019-09-25 13:02 ` Ville Syrjälä
2019-09-20 11:42 ` [PATCH 10/23] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid() Maarten Lankhorst
2019-09-25 5:30 ` Matt Roper
2019-09-25 5:56 ` Matt Roper
2019-09-25 22:09 ` Manasi Navare
2019-09-26 16:00 ` Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 11/23] drm/i915: Try to make bigjoiner work in atomic check Maarten Lankhorst
2019-09-26 3:48 ` Matt Roper
2019-09-30 14:12 ` Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 12/23] drm/i915: Enable big joiner support in enable and disable sequences Maarten Lankhorst
2019-09-26 5:18 ` Matt Roper
2019-09-26 23:54 ` Matt Roper
2019-09-27 8:25 ` Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 13/23] drm/i915: Make hardware readout work on i915 Maarten Lankhorst
2019-09-27 0:49 ` Matt Roper
2019-09-20 11:42 ` [PATCH 14/23] drm/i915: Prepare update_slave() for bigjoiner plane updates Maarten Lankhorst
2019-09-27 3:18 ` Matt Roper
2019-09-20 11:42 ` [PATCH 15/23] drm/i915: Link planes in a bigjoiner configuration Maarten Lankhorst
2019-10-01 16:44 ` Matt Roper
2019-10-01 17:21 ` Ville Syrjälä [this message]
2019-09-20 11:42 ` [PATCH 16/23] drm/i915: Program planes in bigjoiner mode Maarten Lankhorst
2019-09-26 13:06 ` Ville Syrjälä
2019-09-26 15:50 ` Maarten Lankhorst
2019-09-26 16:09 ` Ville Syrjälä
2019-09-26 16:13 ` Maarten Lankhorst
2019-09-26 16:26 ` Ville Syrjälä
2019-09-26 19:11 ` Ville Syrjälä
2019-09-27 8:56 ` Maarten Lankhorst
2019-09-27 14:41 ` Ville Syrjälä
2019-09-27 15:00 ` Ville Syrjälä
2019-09-20 11:42 ` [PATCH 17/23] drm/i915: Add intel_update_bigjoiner handling Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 18/23] drm/i915: Disable FBC in bigjoiner configuration Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 19/23] drm/i915: Prepare atomic plane check for bigjoiner planes Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 20/23] drm/i915: Make prepare_plane_fb() work with " Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 21/23] drm/i915: Make sure watermarks work correctly with bigjoiner as well Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 22/23] drm/i915: Add debugfs dumping for bigjoiner Maarten Lankhorst
2019-09-20 11:42 ` [PATCH 23/23] HAX to make it work on the icelake test system Maarten Lankhorst
2019-09-20 14:52 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/23] drm/i915/dp: Fix dsc bpp calculations, v2 Patchwork
2019-09-20 14:59 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-09-20 15:16 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-20 16:38 ` [Intel-gfx] [PATCH 01/23] " Ville Syrjälä
2019-09-23 12:52 ` [PATCH] drm/i915/dp: Fix dsc bpp calculations, v3 Maarten Lankhorst
2019-09-23 13:03 ` Ville Syrjälä
2019-09-23 13:03 ` Ville Syrjälä
2019-09-23 14:49 ` [PATCH] drm/i915/dp: Fix dsc bpp calculations, v4 Maarten Lankhorst
2019-09-23 14:50 ` Maarten Lankhorst
2019-09-23 14:57 ` Ville Syrjälä
2019-09-23 15:56 ` Manasi Navare
2019-09-23 15:56 ` [Intel-gfx] " Ville Syrjälä
2019-09-23 14:22 ` [Intel-gfx] [PATCH] drm/i915/dp: Fix dsc bpp calculations, v3 kbuild test robot
2019-09-23 14:22 ` kbuild test robot
2019-09-23 15:53 ` Manasi Navare
2019-09-21 12:06 ` [PATCH 01/23] drm/i915/dp: Fix dsc bpp calculations, v2 Sasha Levin
2019-09-21 15:22 ` ✗ Fi.CI.IGT: failure for series starting with [01/23] " Patchwork
2019-09-23 10:43 ` Maarten Lankhorst
2019-09-23 19:10 ` ✗ Fi.CI.BUILD: failure for series starting with drm/i915/dp: Fix dsc bpp calculations, v4. (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191001172106.GR1208@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.