Re: [PATCH 2/4] drm/i915: scramble reset support for DP port CRC on g4x

[Damien Lespiau <damien.lespiau@intel.com>]

On Fri, Nov 01, 2013 at 10:50:21AM +0100, Daniel Vetter wrote:
> +	/* DP port CRC needs slight spec mis-compliance to get stable CRCs. */

We could add a slighty better explanation. How about:

/*
 * 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_scramble_reset) {

so then maybe rename this with 'need_stable_symbols'

> +		uint32_t tmp = I915_READ(PORT_DFT2_G4X);
> +
> +		WARN_ON(!IS_G4X(dev));
> +
> +		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;
>  }
>  
> +static void g4x_undo_pipe_scramble_reset(struct drm_device *dev,
> +					 enum pipe pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t 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);
> +	}
> +}

It took me a bit of time to realize that you were only clearing the
"DC-Balance reset once a frame" bit when you're sure that we have no
pipe CRC needing stable symbols running at all on either pipe. And this
by using the SCRAMBLER_RESET bits in DFT2 as the state tracker.

Maybe add a small comment here to avoid those 20s of head scratching
when reading that code again in a bit.

-- 
Damien