Re: [PATCH 10/21 v2] drm/i915: Helper function to detach a scaler from a plane or crtc

[Matt Roper <matthew.d.roper@intel.com>]

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@lists.freedesktop.org; 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@lists.freedesktop.org; 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@intel.com>
> > > > > ---
> > > > >  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.  :-)


Matt


-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx