Re: [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK

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

On Fri, 2017-03-03 at 14:23 +0200, Ander Conselvan De Oliveira wrote:
> On Tue, 2017-02-28 at 18:57 -0800, Dhinakaran Pandiyan wrote:
> > 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. BXT and GLK both have cdclk frequencies that are less han 192 MHz,
> > so apply the check conditionally for these platforms.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 8fc0f72..89027fa 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1444,6 +1444,8 @@ static int intel_min_cdclk(struct drm_atomic_state *state)
> >  		struct intel_crtc_state *crtc_state;
> >  
> >  		crtc_state = to_intel_crtc_state(cstate);
> > +		if (!crtc_state->has_audio)
> > +			continue;
> >  
> >  		/* According to BSpec, "Do not use DisplayPort with CDCLK less
> >  		 * than 432 MHz, audio enabled, port width x4, and link rate
> > @@ -1452,7 +1454,6 @@ static int intel_min_cdclk(struct drm_atomic_state *state)
> >  		 * for GLK is at 316.8 MHz
> >  		 */
> >  		if (intel_crtc_has_dp_encoder(crtc_state) &&
> > -		    crtc_state->has_audio &&
> >  		    crtc_state->port_clock >= 540000 &&
> >  		    crtc_state->lane_count == 4) {
> >  			if (IS_GEMINILAKE(dev_priv))
> > @@ -1460,6 +1461,13 @@ static int intel_min_cdclk(struct drm_atomic_state *state)
> >  			else if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> >  				min_cdclk = 432000;
> >  		}
> > +
> > +		/* 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.
> > +		 */
> > +		if (IS_BROXTON(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			min_cdclk = max(min_cdclk, 2 * 96000);
> 
> Hmm, my assumption from reviewing patch 1 of "this returns a valid cdclk" is
> broken here. Maybe with the idea of splitting the calc_cdclk_state() in two from
> my comment to Paulo's series this could be just:
> 
> 	min_cdclk = max(min_cdcdlk, choose_cdclk(2 * 96000));
> 
> ?
> 
In this case, we'd have to call <platform>_calc_cdclk() twice? Once for
finding a valid min. cdclk and then again to find the right cdclk for a
given pixel rate. 

> On second thought, it does make sense for this kind of platform specific
> restrictions to be dealt with in platform specific hook, so duplicating that in
> calc_cdclk_state() is not that bad?
> 

I thought of that, but with comments and long conditional checks, it'd
look ugly to duplicate this code in bdw, skl, bxt, glk specific hooks.

-DK

> Ander
> 
> 
> >  	}
> >  
> >  	return min_cdclk;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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