Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2019-02-28 at 17:14 -0800, Souza, Jose wrote:
> On Thu, 2019-02-28 at 17:06 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote:
> > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > Other features like PSR2 also needs to be disabled while
> > > > > getting
> > > > > CRC
> > > > > so lets rename ips_force_disable to crc_enabled, drop all
> > > > > this
> > > > > checks
> > > > > for pipe A and HSW and BDW and make it generic and
> > > > > hsw_compute_ips_config() will take care of all the checks
> > > > > removed
> > > > > from here.
> > > > > 
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c  | 10 +++++--
> > > > >  drivers/gpu/drm/i915/intel_drv.h      |  3 +-
> > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++--------
> > > > > ----
> > > > > ------
> > > > >  3 files changed, 24 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 816e8f124b3b..328967c642b3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -6751,7 +6751,13 @@ static bool
> > > > > hsw_compute_ips_config(struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  	if (!hsw_crtc_state_ips_capable(crtc_state))
> > > > >  		return false;
> > > > >  
> > > > > -	if (crtc_state->ips_force_disable)
> > > > > +	/*
> > > > > +	 * When IPS gets enabled, the pipe CRC changes. Since
> > > > > IPS gets
> > > > > +	 * enabled and disabled dynamically based on package C
> > > > > states,
> > > > > +	 * user space can't make reliable use of the CRCs, so
> > > > > let's
> > > > > just
> > > > > +	 * completely disable it.
> > > > > +	 */
> > > > > +	if (crtc_state->crc_enabled)
> > > > >  		return false;
> > > > 
> > > > Hmm. I was wondering how we even manage to pass the state
> > > > checker
> > > > with
> > > > the current code. But apparently we don't have state checking
> > > > for
> > > > IPS.
> > > > I would suggest moving this into hsw_compute_ips_config() and
> > > > then
> > > > adding the state checker (for HSW only though since BDW can't
> > > > do
> > > > the
> > > > readout).
> > > > 
> > > > >  
> > > > >  	/* IPS should be fine as long as at least one plane is
> > > > > enabled.
> > > > > */
> > > > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  	saved_state->shared_dpll = crtc_state->shared_dpll;
> > > > >  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > > >  	saved_state->pch_pfit.force_thru = crtc_state-
> > > > > > pch_pfit.force_thru;
> > > > > 
> > > > > -	saved_state->ips_force_disable = crtc_state-
> > > > > > ips_force_disable;
> > > > > 
> > > > > +	saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > >  	if (IS_G4X(dev_priv) ||
> > > > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > >  		saved_state->wm = crtc_state->wm;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 5412373e2f98..2be64529e4a2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > > >  	struct intel_link_m_n fdi_m_n;
> > > > >  
> > > > >  	bool ips_enabled;
> > > > > -	bool ips_force_disable;
> > > > > +
> > > > > +	bool crc_enabled;
> > > > >  
> > > > >  	bool enable_fbc;
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > > > intel_pipe_crc_source *source,
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private
> > > > > *dev_priv,
> > > > > -			      bool enable)
> > > > > +static void
> > > > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv,
> > > > > struct
> > > > > drm_crtc *crtc,
> > > > 
> > > > Just pass in the intel_crtc
> > > 
> > > Okay
> > > 
> > > > > +		       bool enable)
> > > > >  {
> > > > >  	struct drm_device *dev = &dev_priv->drm;
> > > > > -	struct intel_crtc *crtc =
> > > > > intel_get_crtc_for_pipe(dev_priv,
> > > > > PIPE_A);
> > > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > 
> > > > and then we don't have to have an ugly name for this.
> > > > 
> > > > Also pasing in dev_priv is redundant when you're already
> > > > passing
> > > > in
> > > > the
> > > > crtc.
> > > > 
> > > 
> > > okay
> > > 
> > > > The function name isn't super descriptive. It makes me think
> > > > we're
> > > > preparing for CRC capture, when in fact it just adds/removes
> > > > the
> > > > w/as.
> > > 
> > > What about: intel_crtc_crc_workarounds_setup()?
> > > 
> > > > >  	struct intel_crtc_state *pipe_config;
> > > > >  	struct drm_atomic_state *state;
> > > > >  	struct drm_modeset_acquire_ctx ctx;
> > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	state->acquire_ctx = &ctx;
> > > > >  
> > > > >  retry:
> > > > > -	pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > > > +	pipe_config = intel_atomic_get_crtc_state(state,
> > > > > intel_crtc);
> > > > >  	if (IS_ERR(pipe_config)) {
> > > > >  		ret = PTR_ERR(pipe_config);
> > > > >  		goto put_state;
> > > > >  	}
> > > > >  
> > > > > -	if (HAS_IPS(dev_priv)) {
> > > > > -		/*
> > > > > -		 * When IPS gets enabled, the pipe CRC changes.
> > > > > Since
> > > > > IPS gets
> > > > > -		 * enabled and disabled dynamically based on
> > > > > package C
> > > > > states,
> > > > > -		 * user space can't make reliable use of the
> > > > > CRCs, so
> > > > > let's just
> > > > > -		 * completely disable it.
> > > > > -		 */
> > > > > -		pipe_config->ips_force_disable = enable;
> > > > > -	}
> > > > > +	pipe_config->crc_enabled = enable;
> > > > >  
> > > > > -	if (IS_HASWELL(dev_priv)) {
> > > > > +	if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > > {
> > > > >  		pipe_config->pch_pfit.force_thru = enable;
> > > > >  		if (pipe_config->cpu_transcoder ==
> > > > > TRANSCODER_EDP &&
> > > > >  		    pipe_config->pch_pfit.enabled != enable)
> > > > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
> > > > > *dev_priv,
> > > > >  				enum pipe pipe,
> > > > >  				enum intel_pipe_crc_source
> > > > > *source,
> > > > > -				u32 *val,
> > > > > -				bool set_wa)
> > > > > +				u32 *val)
> > > > >  {
> > > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > > >  		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_SPRITE_IVB;
> > > > >  		break;
> > > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > -		if (set_wa && (IS_HASWELL(dev_priv) ||
> > > > > -		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > > > -			hsw_pipe_A_crc_wa(dev_priv, true);
> > > > > -
> > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_PF_IVB;
> > > > >  		break;
> > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  
> > > > >  static int get_new_crc_ctl_reg(struct drm_i915_private
> > > > > *dev_priv,
> > > > >  			       enum pipe pipe,
> > > > > -			       enum intel_pipe_crc_source
> > > > > *source, u32
> > > > > *val,
> > > > > -			       bool set_wa)
> > > > > +			       enum intel_pipe_crc_source
> > > > > *source, u32
> > > > > *val)
> > > > >  {
> > > > >  	if (IS_GEN(dev_priv, 2))
> > > > >  		return i8xx_pipe_crc_ctl_reg(source, val);
> > > > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > > >  		return ilk_pipe_crc_ctl_reg(source, val);
> > > > >  	else if (INTEL_GEN(dev_priv) < 9)
> > > > > -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val, set_wa);
> > > > > +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val);
> > > > >  	else
> > > > >  		return skl_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val);
> > > > >  }
> > > > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct
> > > > > drm_crtc
> > > > > *crtc, const char *source_name)
> > > > >  		return -EIO;
> > > > >  	}
> > > > >  
> > > > > -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > > &source, &val,
> > > > > true);
> > > > > +	intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > > +
> > > > 
> > > > Aren't we potentially corrupting the last CRC(s) by turning off
> > > > the
> > > > w/as
> > > > before disbling CRC capture?
> > > 
> > > It will only be NULL when uses closes the file descriptor to CRC
> > > data,
> > > so even if its corrupted user will never get those CRCs.
> > 
> > Even if the userspace reads from another thread? Probably not a
> > common
> > use case though.
> 
> It is called with NULL from the release() hook, so when all
> filedescriptors to this file is closed.
> 
> But anyways the drm crc code only allows one open() of this file at
> time, so if other thread tries to read it after the main thread have
> closed it it will fail before get into any drm code.
> 
Makes sense, thanks.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux