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

[Ander Conselvan De Oliveira]

On Tue, 2017-04-04 at 10:27 +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
> 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??

If someone else needs to deal with this six months or a year from now and you,
me and Jani are nowhere to be found, the code or commit message should still
have enough information to let that person assess whether it is safe to remove
the workaround or not. Having the model number just makes it easier to determine
what real world scenario triggers this problem, so it is easier to test a fix.

Ander

> 
> > 
> > 
> > > 
> > > 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