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 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.


> 
> > 
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx