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

["Konduru, Chandra" <chandra.konduru@intel.com>]

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

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

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