[Intel-gfx] [PATCH] drm/i915/glk: limit pixel clock to 99% of cdclk workaround

Tue Apr 4 11:53:42 UTC 2017

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Tuesday, April 4, 2017 5:09 PM
> To: Chauhan, Madhav <madhav.chauhan at intel.com>
> Cc: Nikula, Jani <jani.nikula at intel.com>; Ander Conselvan De Oliveira
> <conselvan2 at gmail.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/glk: limit pixel clock to 99% of cdclk
> workaround
> 
> On Tue, Apr 04, 2017 at 10:27:57AM +0000, Chauhan, Madhav wrote:
> > > -----Original Message-----
> > > From: Nikula, Jani
> > > Sent: Tuesday, April 4, 2017 3:48 PM
> > > To: Ander Conselvan De Oliveira <conselvan2 at gmail.com>; intel-
> > > gfx at lists.freedesktop.org
> > > Cc: Chauhan, Madhav <madhav.chauhan at intel.com>; Ville Syrjälä
> > > <ville.syrjala at linux.intel.com>
> > > Subject: Re: [PATCH] drm/i915/glk: limit pixel clock to 99% of cdclk
> > > workaround
> > >
> > > On Tue, 04 Apr 2017, Ander Conselvan De Oliveira
> > > <conselvan2 at gmail.com>
> > > wrote:
> > > > On Tue, 2017-04-04 at 11:40 +0300, Jani Nikula wrote:
> > > >> On Tue, 04 Apr 2017, Ander Conselvan De Oliveira
> > > <conselvan2 at gmail.com> wrote:
> > > >> > On Tue, 2017-04-04 at 11:15 +0300, Jani Nikula wrote:
> > > >> > > From: Madhav Chauhan <madhav.chauhan at intel.com>
> > > >> > >
> > > >> > > As per BSPEC, valid cdclk values for glk are 79.2, 158.4, 316.8 Mhz.
> > > >> > > Practically we can achive only 99% of these cdclk values (HW
> > > >> > > team checking on this). So cdclk should be calculated for the
> > > >> > > given pixclk as per that otherwise it may lead to screen
> > > >> > > corruption for some
> > > scenarios.
> > > >> > >
> > > >> > > v2: Rebased to new CDLCK code framework
> > > >> > > v3: Addressed review comments from Ander/Jani
> > > >> > >     - Add comment in code about 99% usage of CDCLK
> > > >> > >     - Calculate max dot clock as well with 99% limit
> > > >> > > v4 by Jani:
> > > >> > >     - drop superfluous whitespace change
> > > >> > >     - rewrite code comments to clarify
> > > >> > >
> > > >> > > Cc: Ander Conselvan de Oliveira
> > > >> > > <ander.conselvan.de.oliveira at intel.com>
> > > >> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > >> > > Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
> > > >> > > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > > >> > > ---
> > > >> > >  drivers/gpu/drm/i915/intel_cdclk.c | 16 +++++++++++++---
> > > >> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > >> > >
> > > >> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > >> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > >> > > index dd3ad52b7dfe..763010f8ad89 100644
> > > >> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > >> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > >> > > @@ -1071,9 +1071,15 @@ static int bxt_calc_cdclk(int
> > > >> > > max_pixclk)
> > > >> > >
> > > >> > >  static int glk_calc_cdclk(int max_pixclk)  {
> > > >> > > -	if (max_pixclk > 2 * 158400)
> > > >> > > +	/*
> > > >> > > +	 * FIXME: Avoid using a pixel clock that is more than 99% of
> the cdclk
> > > >> > > +	 * as a temporary workaround. Use a higher cdclk instead.
> > > >> > > +(Note that
> > > >> >
> > > >> > Temporary workaround for what? Neither the comment nor the
> > > >> > commit message explicitly lists the scenario that triggers this issue.
> > > >>
> > > >> Frankly I did not know what to write.
> > > >
> > > >
> > > >> There are issues with pixel clocks near cdclk that shouldn't
> > > >> happen. People are looking into this, but in the mean time dodge
> > > >> the issues by using higher cdclk. The issue should not be encoder
> > > >> specific, but in practice this has only been seen with DSI
> > > >> because there were some modes with pixel clocks that are near the
> > > >> cdclk.
> > > >
> > > > The above plus the model number of the DSI panel with which the
> > > > issue has been seen would be good enough IMO.
> > >
> > > Madhav?
> >
> > Issue is generic (valid for DP/HDMI) not just MIPI DSI specific.
> > Particular DSI panel exposing this issue as described below:
> > 1. For XXXX panel (19X12) required pixclk is 157100 KHZ
> 
> And what is the actual pixel clock we end up using? As I've mentioned before
> our code is buggy because it assumes we can get any arbitrary clock
> frequency from the PLL. That is not generally true.
> 
> So I'd like to know if the problem really is that we can't reach 100% of cdclk,
> or if we just end up with a pixel clock that slightly higher than what we asked
> for and thus exceeds 100% of cdclk.

AFAIK (from testing, GOP team inputs), problem is that we can't reach 100%CDCLK on GLK. 
In this scenario calculated/required pixel clock is 157100 KHZ  (might be higher due to buggy code as you mentioned)
which should be handled very well if we set CDCLK 79200 KHZ but causes panel corruption.

> 
> And if the problem is really that we can't reach 100% of cdclk, then why are
> we limiting this to GLK only? 

From discussion, looks like issue is specific to GLK platform. 
HW team is debugging this issue.

> 
> > 2. glk_calc_cdclk returns 79200 KHZ for this pixclk. For 2PPC it will
> > be 158400 KHZ 3. Practically 100% of the cdclk can’t be achieved, so 99% of
> 158400 KHZ  = 156816 which is less than the desired pixlclk.
> >      This causes the corruption.
> >
> > So why do we need to mention a particular panel??
> >
> > >
> > >
> > > >
> > > > Ander
> > > >
> > > >
> > > >> > With that fixed,
> > > >> >
> > > >> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
> > > >> >
> > > >> > > +	 * intel_compute_max_dotclk() limits the max pixel clock to
> > > >> > > +99% of
> > > max
> > > >> > > +	 * cdclk.)
> > > >> > > +	 */
> > > >> > > +	if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
> > > >> > >  		return 316800;
> > > >> > > -	else if (max_pixclk > 2 * 79200)
> > > >> > > +	else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
> > > >> > >  		return 158400;
> > > >> > >  	else
> > > >> > >  		return 79200;
> > > >> > > @@ -1664,7 +1670,11 @@ static int
> > > >> > > intel_compute_max_dotclk(struct
> > > drm_i915_private *dev_priv)
> > > >> > >  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> > > >> > >
> > > >> > >  	if (IS_GEMINILAKE(dev_priv))
> > > >> > > -		return 2 * max_cdclk_freq;
> > > >> > > +		/*
> > > >> > > +		 * FIXME: Limiting to 99% as a temporary
> workaround. See
> > > >> > > +		 * glk_calc_cdclk() for details.
> > > >> > > +		 */
> > > >> > > +		return 2 * max_cdclk_freq * 99 / 100;
> > > >> > >  	else if (INTEL_INFO(dev_priv)->gen >= 9 ||
> > > >> > >  		 IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > >> > >  		return max_cdclk_freq;
> > > >>
> > > >>
> > >
> > > --
> > > Jani Nikula, Intel Open Source Technology Center
> 
> --
> Ville Syrjälä
> Intel OTC