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

Wed Mar 25 14:28:16 PDT 2015

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Wednesday, March 25, 2015 2:14 PM
> To: Konduru, Chandra
> Cc: intel-gfx at 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 at 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 at 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
> 
> >
> > >
> > >
> > > Matt
> > >
> > > > +	}
> > > > +}
> > > > +
> > > >  static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > > >  					 struct drm_framebuffer *fb,
> > > >  					 int x, int y)
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index a9d787d..f25d14d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1141,6 +1141,7 @@ void intel_modeset_preclose(struct
> > > > drm_device *dev, struct drm_file *file);  int
> > > > skl_update_scaler_users(struct intel_crtc
> > > *intel_crtc,
> > > >  	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
> > > >  	struct intel_plane_state *plane_state, int force_detach);
> > > > +void skl_detach_scaler(struct drm_crtc *crtc, struct drm_plane
> > > > +*plane);
> > > >
> > > >  /* intel_dp.c */
> > > >  void intel_dp_init(struct drm_device *dev, int output_reg, enum
> > > > port port);
> > > > --
> > > > 1.7.9.5
> > > >
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development Intel Corporation
> > > (916) 356-2795
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795