Re: [PATCH v2 2/2] drm/i915: Implement cdclk restrictions based on Azalia BCLK

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2017-03-07 às 16:12 -0800, Dhinakaran Pandiyan escreveu:
> According to BSpec, "The CD clock frequency must be at least twice
> the
> frequency of the Azalia BCLK." and BCLK is configured to 96 MHz by
> default. This check is needed because BXT and GLK support cdclk
> frequencies less than 192 MHz.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index e8c1181..7b1ac1d 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1458,6 +1458,18 @@ static int
> bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>  			pixel_rate = max(432000, pixel_rate);
>  	}
>  
> +	/* According to BSpec, "The CD clock frequency must be at
> least twice
> +	 * the frequency of the Azalia BCLK." and BCLK is 96 MHz by
> default.
> +	 * The check for GLK has to be adjusted as the platform can
> output
> +	 * two pixels per clock.
> +	 */
> +	if (crtc_state->has_audio) {
> +		if (IS_GEMINILAKE(dev_priv))
> +			pixel_rate = max(2 * 2 * 96000, pixel_rate);
> +		if (IS_BROXTON(dev_priv))

SKL also documents this in the page that explains the cdclk freq change
sequences. The funny thing is that the minimum CDCLK for SKL seems to
be 308.57, so that's still bigger than 96*2... Anyway, having this for
completeness would probably be good, just in case I'm missing some
detail that's important here.

I'd like to see the SKL addition, but I can live without it in case you
have some better argument, so if you don't send a new version, here's
it:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


Perhaps it would also be interesting to have some sort of macro to
identify the platform(s) that need the magic *2 calculation. A more
confusion-proof version of this function would look like this:

if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
	if (HAS_2_PIXELS_PER_CLOCK(dev_priv))
		pixel_rate = max(2 * 2 * 96000, pixel_rate);
	else
		pixel_rate = max(2 * 96000, pixel_rate);

Maybe we'd be able to use the macro in other places too (I haven't
checked). The new macro would definitely be part of a separate patch on
top of these.



> +			pixel_rate = max(2 * 96000, pixel_rate);
> +	}
> +
>  	return pixel_rate;
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx