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

[Ville Syrjälä]

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.

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

> 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