> -----Original Message-----
> From: Roper, Matthew D
> Sent: Friday, March 27, 2015 5:22 PM
> To: Konduru, Chandra
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 10/21 v2] drm/i915: Helper function to detach a scaler from
> a plane or crtc
>
> On Wed, Mar 25, 2015 at 02:28:16PM -0700, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Roper, Matthew D
> > > Sent: Wednesday, March 25, 2015 2:14 PM
> > > To: Konduru, Chandra
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De
> > > Oliveira, Ander
> > > Subject: Re: [PATCH 10/21 v2] drm/i915: Helper function to detach a
> > > scaler from a plane or crtc
> > >
> > > On Wed, Mar 25, 2015 at 01:14:40PM -0700, Konduru, Chandra wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Roper, Matthew D
> > > > > Sent: Tuesday, March 24, 2015 10:16 PM
> > > > > To: Konduru, Chandra
> > > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan
> > > > > De Oliveira, Ander
> > > > > Subject: Re: [PATCH 10/21 v2] drm/i915: Helper function to
> > > > > detach a scaler from a plane or crtc
> > > > >
> > > > > On Fri, Mar 20, 2015 at 05:04:31PM -0700, Chandra Konduru wrote:
> > > > > > This function is called from commit path of a plane or crtc.
> > > > > > It programs scaler registers to detach (aka. unbinds) scaler
> > > > > > from requested plane or crtc if it isn't in use. It also
> > > > > > resets scaler_id in crtc/plane state.
> > > > > >
> > > > > > v2:
> > > > > > -improved a log message (me)
> > > > > >
> > > > > > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_display.c | 39
> > > > > ++++++++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > > > 2 files changed, 40 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 976bfb1..7150c33 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -2836,6 +2836,45 @@ u32 intel_fb_stride_alignment(struct
> > > > > > drm_device
> > > > > *dev, uint64_t fb_modifier,
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +/*
> > > > > > + * This function detaches (aka. unbinds) a scaler from plane
> > > > > > +or crtc
> > > > >
> > > > > You might want to clarify that detach/unbind refers to the
> > > > > actual hardware programming, not the state calculation. I'm a
> > > > > bit surprised we need this function; I figured we'd just be
> > > > > looping over all scalers at the end of the commit step and
> > > > > programming them to either on or off depending on what the scaling
> state contained.
> > > >
> > > > This is bit tricky and isn't that straight forward.
> > > > Staged scaler state can trigger freeing a scaler that can lead to two
> scenarios:
> > > > 1) freed scaler is not attached to any other user. In this case,
> > > > reg programming needed to update hw. And reset scaler_id to -1 to
> > > > indicate scaler
> > > isn't used.
> > > > Once done, both sw and hw states are in sync.
> > > >
> > > > 2) freed scaler is allocated to someone. In this case, registers
> > > > shouldn't be programmed by previous owner because scaler may be in
> > > > use by new owner.
> > > >
> > > > I think I explained these details in comments already. But will
> > > > check and update if needed.
> > >
> > > I might not have been clear in my earlier email. What I meant was
> > > that I didn't expect scalers to be programmed as part of their "owner's"
> > > programming at all. At the moment you seem to be programming them
> > > in the low-level plane programming functions (skl_update_plane and such).
> > > Instead, I had expected a single loop over each scaler at the very
> > > end of the entire commit process, after you're done with the plane
> programming functions.
> > > The scaler state would already indicate whether the scaler is
> > > supposed to be associated with a plane/crtc (which may or may not be
> > > the same as the previous frame; we don't care) or whether it is unused and
> should be programmed to off.
> > > So basically you would wind up programming all of the scaler
> > > registers on each atomic commit, even if they didn't change, but you
> > > wouldn't have to worry about whether the scaler's owner is changing
> > > or who is responsible for doing the programming of that scaler this
> > > time around --- basically just treat scalers as an independent
> > > resource that have their own programming step at the end of processing a
> CRTC.
> >
> > OK. Now I understand what you meant.
> > In early versions, I tried something similar, but that approach
> > required back pointers from scaler to its owner to program scaler
> > output window coordinates.
> > And it also requires managing back pointer assignment and resetting.
> > It is certainly doable but I didn't see any advantage than current approach.
> >
> > >
> > >
> > > >
> > > > >
> > > > > As I mentioned on a previous patch, these overloaded functions
> > > > > that might operate on a plane or might operate on a CRTC can be
> > > > > a bit confusing, especially when we have multi-nested ternary
> > > > > operators like you
> > > do below.
> > > >
> > > > >
> > > > > > + * if scaler is not in use.
> > > > > > + * It resets scaler_id in plane or crtc
> > > > > > + * To request detach a scaler from crtc, call plane as NULL
> > > > > > +*/ void skl_detach_scaler(struct drm_crtc *crtc, struct drm_plane
> *plane) {
> > > > > > + struct drm_device *dev = crtc->dev;
> > > > > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > + struct intel_crtc_state *crtc_state;
> > > > > > + struct intel_crtc *intel_crtc;
> > > > > > + struct intel_plane *intel_plane;
> > > > > > + struct intel_plane_state *plane_state;
> > > > > > + int *scaler_id;
> > > > > > +
> > > > > > + intel_crtc = to_intel_crtc(crtc);
> > > > > > + intel_plane = plane ? to_intel_plane(plane) : NULL;
> > > > > > + crtc_state = intel_crtc->config;
> > > > > > + plane_state = plane ? to_intel_plane_state(plane->state) :
> > > > > > +NULL;
> > > > > > +
> > > > > > + scaler_id = plane ? (plane_state ? &plane_state->scaler_id :
> NULL) :
> > > > > > + &crtc_state->scaler_state.scaler_id;
> > > > > > +
> > > > > > + if (!scaler_id || (scaler_id && *scaler_id < 0))
> > > > > > + return;
> > > > > > +
> > > > > > + /* if scaler is not in use, free */
> > > > > > + if (!crtc_state->scaler_state.scalers[*scaler_id].in_use) {
> > > > > > + I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe,
> (*scaler_id)), 0);
> > > > > > + I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe,
> (*scaler_id)),
> > > > > 0);
> > > > > > + I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe,
> (*scaler_id)), 0);
> > > > > > + DRM_DEBUG_KMS("Detached and disabled scaler id
> %u.%u
> > > > > from %s:%d\n",
> > > > > > + intel_crtc->pipe, *scaler_id, plane ? "PLANE" :
> "CRTC",
> > > > > > + plane ? plane->base.id : crtc->base.id);
> > > > > > + *scaler_id = -1;
> > > > >
> > > > > This confuses me...why are we updating the state here at the end
> > > > > of the commit step? State should be immutable at this point, right?
> > > >
> > > > As I explained above, valid scaler_id is required. Then scaler_id
> > > > can be set to -
> > > 1.
> > >
> > > The problem is that we're ultimately updating crtc_state from the
> > > 'commit' step here, which violates the atomic design. State
> > > structures are supposed to be immutable during the commit phase
>
> This is still a problem; we really can't go back and update the state structure in a
> function that is ultimately called as part of the 'commit' phase. Also keep in
> mind that locks may have already been dropped before we start the commit
> phase (for non-blocking flips that run in a wq thread).
>
> I'm still unconvinced on the need for the whole "program scaler registers with
> their current owner" design. Above you said:
>
> > OK. Now I understand what you meant.
> > In early versions, I tried something similar, but that approach
> > required back pointers from scaler to its owner to program scaler
> > output window coordinates.
> > And it also requires managing back pointer assignment and resetting.
> > It is certainly doable but I didn't see any advantage than current approach.
>
> When we assign a scaler to a plane or a CRTC in the check phase, why can't we
> just pre-calculate the actual register values like
>
> crtc_state->scaler_state.scalers[i].ctrl
> crtc_state->scaler_state.scalers[i].win_pos
> crtc_state->scaler_state.scalers[i].win_sz
>
> at that point rather than trying to maintain backpointers to planes or crtcs?
> Then at the end of your CRTC commit you could just do the equivalent of:
>
> for each scaler {
> I915_WRITE(SKL_PS_CTRL[i], state->ctrl);
> I915_WRITE(SKL_PS_WIN_POS[i], state->win_pos);
> I915_WRITE(SKL_PS_WIN_SZ[i], state->win_sz);
> }
>
> This eliminates the need for the whole 'detach' concept, avoids the need for
> backpointers to the actual plane/crtc which you were worried about, and brings
> all the programming into one place rather than spreading it across a bunch of
> different planes/crtc functions, which is a lot more complicated logic-wise.
>
> Feel free to tell me I'm stupid if I'm overlooking some obvious stumbling block
> with this approach. :-)
I had a chat with Matt and his concern is when state is duplicated for a
subsequent flip while a non-blocking flip is staged but not committed.
This isn't an issue because the state in crtc is still whatever previously
committed state and not uncommitted but staged state.
Anyway, once full atomic crtc check/commit is in place I can revisit
and make any mods if above update is an issue. For now I think it is fine.
>
>
> Matt
>
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
