Re: [PATCH 08/24] drm/i915: Do not add planes from intel_atomic_setup_scalers.

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

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, June 02, 2015 6:30 PM
> To: Maarten Lankhorst
> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> > This may postpone going to HQ mode until the plane is in the
> > drm_atomic_state if it's not using scaler 0, but it does allow moving
> > intel_atomic_setup_scalers to the crtc check function.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++----------------
> --
> >  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  3 files changed, 39 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> > index 1edd1651c045..a8202fa0daa8 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >
> > -	/*
> > -	 * FIXME: move to crtc atomic check function once this is
> > -	 * more atomic friendly.
> > -	 */
> > -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return ret;
> >  }
> >
> > @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> *dev,
> >  			/* find the plane that set the bit as scaler_user */
> >  			plane = drm_state->planes[i];
> >
> > -			/*
> > -			 * to enable/disable hq mode, add planes that are using
> scaler
> > -			 * into this transaction
> > -			 */
> >  			if (!plane) {
> > -				struct drm_plane_state *state;
> > -				plane = drm_plane_from_index(dev, i);
> > -				state =
> drm_atomic_get_plane_state(drm_state, plane);
> > -				if (IS_ERR(state)) {
> > -					DRM_DEBUG_KMS("Failed to add
> [PLANE:%d] to drm_state\n",
> > -						plane->base.id);
> > -					return PTR_ERR(state);
> > -				}
> > +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> in drm_state\n", plane->base.id);
> > +				continue;
> >  			}
> > -
> >  			intel_plane = to_intel_plane(plane);
> >
> >  			/* plane on different crtc cannot be a scaler user of this
> crtc */
> > @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> *dev,
> >  		}
> >  	}
> >
> > +	/* plane not part of mask must leave hq mode? */
> > +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> > +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> > +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> > +
> > +		intel_crtc->atomic.skl_update_scaler0 =
> > +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> > +	}
> > +
> > +	/* plane not part of mask can enter hq mode? */
> > +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> > +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> PS_SCALER_MODE_HQ) {
> > +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> > +
> > +		intel_crtc->atomic.skl_update_scaler0 =
> > +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> > +	}
> > +
> 
> I don't have access to the hw spec at the moment; is scaler #0 the only
> one that can ever go into HQ mode?  

Yes

> If there isn't a hardware
> requirement about this, then it seems like we're missing the case where
> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> disabled, which should allow scaler 1 to go into HQ mode.

In this case, scaler 0 to be allocated to plane B to operate in HQ mode.

> 
> I guess it's not immediately clear to me why we need to not pull the
> other planes into the transaction.  Is this just to avoid doing some
> extra work for a plane that hasn't changed, or does it cause a problem
> somehow?

Per atomic design, unchanged planes can be added to transaction.
And scaler implementation is using this design feature.
Not sure what the issue here, but we need this feature continue
to available.

> 
> Chandra should probably take a look at this patch as well since he's the
> scaler expert; adding him to the Cc.
> 
> 
> Matt
> 
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 0060784525dc..94101d1f784c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6530,7 +6530,6 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config-
> >base.adjusted_mode;
> > -	int ret;
> >
> >  	/* FIXME should check pixel clock limits on all platforms */
> >  	if (INTEL_INFO(dev)->gen < 4) {
> > @@ -6577,14 +6576,7 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> >  	if (pipe_config->has_pch_encoder)
> >  		return ironlake_fdi_compute_config(crtc, pipe_config);
> >
> > -	/* FIXME: remove below call once atomic mode set is place and all crtc
> > -	 * related checks called from atomic_crtc_check function */
> > -	ret = 0;
> > -	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config-
> >base.state) = %p\n",
> > -		crtc, pipe_config->base.state);
> > -	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static int skylake_get_display_clock_speed(struct drm_device *dev)
> > @@ -11457,7 +11449,10 @@ static bool check_encoder_cloning(struct
> drm_atomic_state *state,
> >  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *crtc_state)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_state *pipe_config =
> > +		to_intel_crtc_state(crtc_state);
> >  	struct drm_atomic_state *state = crtc_state->state;
> >  	int idx = crtc->base.id;
> >  	bool is_crtc_enabled = crtc_state->active;
> > @@ -11476,7 +11471,7 @@ static int intel_atomic_check_crtc(struct
> drm_crtc *crtc,
> >  	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> >  			 idx, was_crtc_enabled, is_crtc_enabled);
> >
> > -	return 0;
> > +	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
> >  }
> >
> >  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> > @@ -13423,6 +13418,18 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc)
> >  		intel_crtc->atomic.evade =
> >  			intel_pipe_update_start(intel_crtc,
> >  						&intel_crtc-
> >atomic.start_vbl_count);
> > +
> > +	if (intel_crtc->atomic.skl_update_scaler0) {
> > +		uint32_t ps_ctrl, ps_win_sz;
> > +
> > +		ps_ctrl = I915_READ(SKL_PS_CTRL(intel_crtc->pipe, 0));
> > +		ps_ctrl &= ~PS_SCALER_MODE_MASK;
> > +		ps_ctrl |= intel_crtc->atomic.skl_update_scaler0 &
> PS_SCALER_MODE_MASK;
> > +		I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, 0), ps_ctrl);
> > +
> > +		ps_win_sz = I915_READ(SKL_PS_WIN_SZ(intel_crtc->pipe, 0));
> > +		I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, 0), ps_win_sz);
> > +	}
> >  }
> >
> >  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> > index 1d9feb8727ea..71ad8a1e13ab 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -487,6 +487,8 @@ struct intel_crtc_atomic_commit {
> >  	bool update_wm;
> >  	unsigned disabled_planes;
> >
> > +	unsigned skl_update_scaler0;
> > +
> >  	/* Sleepable operations to perform after commit */
> >  	unsigned fb_bits;
> >  	bool wait_vblank;
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> 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