Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Mon, 2019-02-18 at 19:57 +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2019 at 09:43:37PM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2019-02-15 at 23:34 +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan
> > > wrote:
> > > > On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > > > > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan
> > > > > wrote:
> > > > > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > 
> > > > > > > DP CRCs don't really work on g4x. If you want any CRCs on
> > > > > > > DP
> > > > > > > you
> > > > > > > must
> > > > > > > select the CRC source before the port is enabled,
> > > > > > > otherwise
> > > > > > > the
> > > > > > > CRC
> > > > > > > source select bits simply ignore any writes to them. And
> > > > > > > once
> > > > > > > the
> > > > > > > port
> > > > > > > is enabled we mustn't change the CRC source select until
> > > > > > > the
> > > > > > > port
> > > > > > > is
> > > > > > > disabled. That almost works, but not quite :( Eventually
> > > > > > > the
> > > > > > > CRC
> > > > > > > source
> > > > > > > select bits get permanently stuck one way or the other,
> > > > > > > and
> > > > > > > after
> > > > > > > that
> > > > > > > a reboot (or possibly a display reset) is needed to get
> > > > > > > working
> > > > > > > CRCs
> > > > > > > on that pipe (not matter which CRC source we try to use).
> > > > > > > 
> > > > > > > Additionally the DFT scrambler reset bits we're trying to
> > > > > > > use
> > > > > > > don't
> > > > > > > seem to exist on g4x. There are some potentially relevant
> > > > > > > looking
> > > > > > > bits
> > > > > > > in the pipe registers, but when I tried it I got stable
> > > > > > > looking
> > > > > > > CRCs
> > > > > > > without setting any bits for this.
> > > > > > > 
> > > > > > > If there is a way to make DP CRCs work reliably on g4x, I
> > > > > > > wasn't
> > > > > > > able to find it. So let's just remove the broken code we
> > > > > > > have.
> > > > > > 
> > > > > > 
> > > > > > I think we can modify i9xx_pipe_crc_auto_source() to pick
> > > > > > "pipe"
> > > > > > CRC
> > > > > > when userspace selects "auto" and the output is DP/eDP.
> > > > > 
> > > > > Nope. Spec says:
> > > > > "Pipe CRC should not be run when Display Port or TV is
> > > > > enabled on
> > > > > this
> > > > >  pipe."
> > > > > 
> > > > > and
> > > > > 
> > > > > "CRC Source Select: These bits select the source of the data
> > > > > to
> > > > > put
> > > > > into
> > > > >  the CRC logic.
> > > > >  000: Pipe A (Not available when DisplayPort or TV is enabled
> > > > > on
> > > > > this
> > > > >  pipe)"
> > > > > 
> > > > 
> > > > After digging through some old specs, I do see this restriction
> > > > for
> > > > gen-4 and VLV, but for some reason not for gen-3 or CHV. 
> > > 
> > > gen3 predates DP (g4x being the first platform that has it). I
> > > don't
> > > think SDVO->DP was ever a thing. SDVO->HDMI did happen but even
> > > that
> > > one is quite rare.
> > 
> > TV? I see TV initialization for a couple of gen-3 platforms but the
> > spec does not say that pipe CRCs are not available.
> 
> Could just be an omission in the spec. I don't think I actually
> tested to see what happens when you try to use the pipe CRC with the
> TV encoder. Presumably it might give you something useful, but it
> certainly wouldn't account for anything done by the TV encoder.
> Not that we normally care about that stuff anyway.

PSR2 might be one of those cases that makes encoder CRCs interesting -
for e.g., compare DDI CRC of a PSR2 SU update region against a
reference. I don't know how we can generate a reference CRC for a SU
update region though.

> 
> > > 
> > > The display engine side of CHV is 99.9% VLV, with a few extra
> > > bits and pieces glued on top.
> > 
> > Thanks for the clarification, the CHV spec for some reason make it
> > a
> > point to specify VLV in paranthesis
> 
> They basically just did 'cp VLVspec CHVspec', and then edited
> it minimally. So you should generally interpret the "DevVLV*"
> to mean "VLV/CHV".

Got it, thanks for explaining :)

-DK

> 
> > 
> > 0000: Pipe C (Not available when DisplayPort or TV is enabled on
> > this
> > pipe) [VLV]
> > 
> > > 
> > > > 
> > > > There is no good choice for "auto" other than DP and since DP
> > > > does
> > > > not
> > > > work, returning -EINVAL makes sense.
> > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > 
> > > > > Though I must admit I've never actually tried it to see what
> > > > > actually
> > > > > happens.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Ville Syrjälä <
> > > > > > > ville.syrjala@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++---------
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > > --
> > > > > > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > index fe0ff89b980b..66bb7b031537 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > @@ -191,8 +191,6 @@ static int
> > > > > > > i9xx_pipe_crc_ctl_reg(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  				 enum intel_pipe_crc_source
> > > > > > > *source,
> > > > > > >  				 u32 *val)
> > > > > > >  {
> > > > > > > -	bool need_stable_symbols = false;
> > > > > > > -
> > > > > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > > > > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv,
> > > > > > > pipe,
> > > > > > > source);
> > > > > > >  		if (ret)
> > > > > > > @@ -208,56 +206,23 @@ static int
> > > > > > > i9xx_pipe_crc_ctl_reg(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  			return -EINVAL;
> > > > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_TV_PRE;
> > > > > > >  		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_B_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_C_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_D_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > >  		*val = 0;
> > > > > > >  		break;
> > > > > > >  	default:
> > > > > > > +		/*
> > > > > > > +		 * The DP CRC source doesn't work on g4x.
> > > > > > > +		 * It can be made to work to some degree by
> > > > > > > selecting
> > > > > > > +		 * the correct CRC source before the port is
> > > > > > > enabled,
> > > > > > > +		 * and not touching the CRC source bits again
> > > > > > > until
> > > > > > > +		 * the port is disabled. But even then the bits
> > > > > > > +		 * eventually get stuck and a reboot is needed
> > > > > > > to get
> > > > > > > +		 * working CRCs on the pipe again. Let's simply
> > > > > > > +		 * refuse to use DP CRCs on g4x.
> > > > > > > +		 */z
> > > > > > >  		return -EINVAL;
> > > > > > >  	}
> > > > > > >  
> > > > > > > -	/*
> > > > > > > -	 * When the pipe CRC tap point is after the transcoders
> > > > > > > we need
> > > > > > > -	 * to tweak symbol-level features to produce a
> > > > > > > deterministic
> > > > > > > series of
> > > > > > > -	 * symbols for a given frame. We need to reset those
> > > > > > > features
> > > > > > > only once
> > > > > > > -	 * a frame (instead of every nth symbol):
> > > > > > > -	 *   - DC-balance: used to ensure a better clock
> > > > > > > recovery from
> > > > > > > the data
> > > > > > > -	 *     link (SDVO)
> > > > > > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > > > > > -	 */
> > > > > > > -	if (need_stable_symbols) {
> > > > > > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > > -
> > > > > > > -		WARN_ON(!IS_G4X(dev_priv));
> > > > > > > -
> > > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > > -			   I915_READ(PORT_DFT_I9XX) |
> > > > > > > DC_BALANCE_RESET);
> > > > > > > -
> > > > > > > -		if (pipe == PIPE_A)
> > > > > > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > > > > -		else
> > > > > > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > > > > -
> > > > > > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -	}
> > > > > > > -
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -282,24 +247,6 @@ static void
> > > > > > > vlv_undo_pipe_scramble_reset(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > > > > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > > > > > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -
> > > > > > > -}
> > > > > > > -
> > > > > > > -static void g4x_undo_pipe_scramble_reset(struct
> > > > > > > drm_i915_private
> > > > > > > *dev_priv,
> > > > > > > -					 enum pipe pipe)
> > > > > > > -{
> > > > > > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > > -
> > > > > > > -	if (pipe == PIPE_A)
> > > > > > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > > > > -	else
> > > > > > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > > > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -
> > > > > > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > > -			   I915_READ(PORT_DFT_I9XX) &
> > > > > > > ~DC_BALANCE_RESET);
> > > > > > > -	}
> > > > > > >  }
> > > > > > >  
> > > > > > >  static int ilk_pipe_crc_ctl_reg(enum
> > > > > > > intel_pipe_crc_source
> > > > > > > *source,
> > > > > > > @@ -485,9 +432,6 @@ static int
> > > > > > > i9xx_crc_source_valid(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  	switch (source) {
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > >  		return 0;
> > > > > > >  	default:
> > > > > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct
> > > > > > > drm_crtc
> > > > > > > *crtc, const char *source_name)
> > > > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > > >  
> > > > > > >  	if (!source) {
> > > > > > > -		if (IS_G4X(dev_priv))
> > > > > > > -			g4x_undo_pipe_scramble_reset(dev_priv,
> > > > > > > crtc-
> > > > > > > > index);
> > > > > > > 
> > > > > > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > > +		if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > >  			vlv_undo_pipe_scramble_reset(dev_priv,
> > > > > > > crtc-
> > > > > > > > index);
> > > > > > > 
> > > > > > >  		else if ((IS_HASWELL(dev_priv) ||
> > > > > > >  			  IS_BROADWELL(dev_priv)) && crtc-
> > > > > > > > index ==
> > > > > > > 
> > > > > > > PIPE_A)
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx