diff mbox

[v2,1/3] drm/i915: Fix up CNL cdclk related limits

Message ID 20170710130211.16859-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala July 10, 2017, 1:02 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Follow the GLK path when computing cdclk and related limits. CNL
pipes also produce two pixels per clock, so that's what we should
really use. However for the purposes of pixel rate calculations we
will assume one pixel per clock to keep the voltage higher, at least
until the missing voltage scaling for DDI clocks is implemented.

For the HBR2 vs. audio issue the limit should more correctly be 336
MHz, but the GLK limit of 316.8 MHz works just as well and results
in picking at least 336 MHz. Also toss in some related w/a numbers.

v2: Assume 1 pixel per clock for the purposes of max pixel rate
    calculation until DDI clock voltage scaling is handled

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Dhinakaran Pandiyan July 10, 2017, 5:34 p.m. UTC | #1
On Mon, 2017-07-10 at 16:02 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 

> Follow the GLK path when computing cdclk and related limits. CNL

> pipes also produce two pixels per clock, so that's what we should

> really use. However for the purposes of pixel rate calculations we

> will assume one pixel per clock to keep the voltage higher, at least

> until the missing voltage scaling for DDI clocks is implemented.

> 


Does the lack of  correct voltage scaling implementation affect only
intel_compute_max_dotclk()? i.e., allowing a pixel rate of
2*max_cdclk_freq? Or does it mean cnl_calc_cdclk() cannot take into
account pixel_rate <= 2*cdclk_freq for any frequency?


With this patch, 
bdw_adjust_min_pipe_pixel_rate() compares pixel_rate to 2*cdclk
cnl_calc_cdclk() compares pixel_rate to 1*cdclk.
Isn't that a discrepancy?


> For the HBR2 vs. audio issue the limit should more correctly be 336

> MHz, but the GLK limit of 316.8 MHz works just as well and results

> in picking at least 336 MHz. Also toss in some related w/a numbers.


In this case, _adjust_min_pipe_pixel_rate() will return pixel_rate as
633.6 MHz, followed by cnl_calc_cdclk() returning 528 MHz cdclk. But,
isn't the correct workaround cdclk 336 MHz?


> 

> v2: Assume 1 pixel per clock for the purposes of max pixel rate

>     calculation until DDI clock voltage scaling is handled

> 

> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------

>  1 file changed, 14 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c

> index 1241e5891b29..4b8eb6a7d852 100644

> --- a/drivers/gpu/drm/i915/intel_cdclk.c

> +++ b/drivers/gpu/drm/i915/intel_cdclk.c

> @@ -1752,12 +1752,13 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,

>  	    crtc_state->has_audio &&

>  	    crtc_state->port_clock >= 540000 &&

>  	    crtc_state->lane_count == 4) {

> -		if (IS_CANNONLAKE(dev_priv))

> -			pixel_rate = max(316800, pixel_rate);

> -		else if (IS_GEMINILAKE(dev_priv))

> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {

> +			/* Display WA #1145: glk,cnl */

>  			pixel_rate = max(2 * 316800, pixel_rate);

> -		else

> +		} else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {

> +			/* Display WA #1144: skl,bxt */

>  			pixel_rate = max(432000, pixel_rate);

> +		}

>  	}

>  

>  	/* According to BSpec, "The CD clock frequency must be at least twice

> @@ -1766,7 +1767,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,

>  	 * two pixels per clock.

>  	 */

>  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {

> -		if (IS_GEMINILAKE(dev_priv))

> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))

>  			pixel_rate = max(2 * 2 * 96000, pixel_rate);

>  		else

>  			pixel_rate = max(2 * 96000, pixel_rate);

> @@ -1999,7 +2000,14 @@ 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))

> +	if (INTEL_GEN(dev_priv) >= 10)

> +		/*

> +		 * FIXME: Allow '2 * max_cdclk_freq'

> +		 * once DDI clock voltage requirements are

> +		 * handled correctly.

> +		 */

> +		return max_cdclk_freq;

> +	else if (IS_GEMINILAKE(dev_priv))

>  		/*

>  		 * FIXME: Limiting to 99% as a temporary workaround. See

>  		 * glk_calc_cdclk() for details.
Rodrigo Vivi July 10, 2017, 5:34 p.m. UTC | #2
cool, with

v2 of patch 1
v2 of patch 2
patch 3

display works properly here on cnl.

On Mon, Jul 10, 2017 at 6:02 AM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Follow the GLK path when computing cdclk and related limits. CNL
> pipes also produce two pixels per clock, so that's what we should
> really use. However for the purposes of pixel rate calculations we
> will assume one pixel per clock to keep the voltage higher, at least
> until the missing voltage scaling for DDI clocks is implemented.
>
> For the HBR2 vs. audio issue the limit should more correctly be 336
> MHz, but the GLK limit of 316.8 MHz works just as well and results
> in picking at least 336 MHz. Also toss in some related w/a numbers.
>
> v2: Assume 1 pixel per clock for the purposes of max pixel rate
>     calculation until DDI clock voltage scaling is handled
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 1241e5891b29..4b8eb6a7d852 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1752,12 +1752,13 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>             crtc_state->has_audio &&
>             crtc_state->port_clock >= 540000 &&
>             crtc_state->lane_count == 4) {
> -               if (IS_CANNONLAKE(dev_priv))
> -                       pixel_rate = max(316800, pixel_rate);
> -               else if (IS_GEMINILAKE(dev_priv))
> +               if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> +                       /* Display WA #1145: glk,cnl */
>                         pixel_rate = max(2 * 316800, pixel_rate);
> -               else
> +               } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> +                       /* Display WA #1144: skl,bxt */
>                         pixel_rate = max(432000, pixel_rate);
> +               }
>         }
>
>         /* According to BSpec, "The CD clock frequency must be at least twice
> @@ -1766,7 +1767,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
>          * two pixels per clock.
>          */
>         if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> -               if (IS_GEMINILAKE(dev_priv))
> +               if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>                         pixel_rate = max(2 * 2 * 96000, pixel_rate);
>                 else
>                         pixel_rate = max(2 * 96000, pixel_rate);
> @@ -1999,7 +2000,14 @@ 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))
> +       if (INTEL_GEN(dev_priv) >= 10)
> +               /*
> +                * FIXME: Allow '2 * max_cdclk_freq'
> +                * once DDI clock voltage requirements are
> +                * handled correctly.
> +                */
> +               return max_cdclk_freq;
> +       else if (IS_GEMINILAKE(dev_priv))
>                 /*
>                  * FIXME: Limiting to 99% as a temporary workaround. See
>                  * glk_calc_cdclk() for details.

Are you sure we don't want this workaround also? With so similar
display engines I wonder if we would end with similar issues.
But I'm just asking... because honestly I didn't check that 99%
workaround closely enough yet...

The rest of the patch makes sense for me and works so feel free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

rv-b only for this test for now, but tested-by you could use in all 3
patches mentioned above...


> --
> 2.13.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala July 10, 2017, 5:55 p.m. UTC | #3
On Mon, Jul 10, 2017 at 10:34:22AM -0700, Rodrigo Vivi wrote:
> cool, with
> 
> v2 of patch 1
> v2 of patch 2
> patch 3
> 
> display works properly here on cnl.
> 
> On Mon, Jul 10, 2017 at 6:02 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Follow the GLK path when computing cdclk and related limits. CNL
> > pipes also produce two pixels per clock, so that's what we should
> > really use. However for the purposes of pixel rate calculations we
> > will assume one pixel per clock to keep the voltage higher, at least
> > until the missing voltage scaling for DDI clocks is implemented.
> >
> > For the HBR2 vs. audio issue the limit should more correctly be 336
> > MHz, but the GLK limit of 316.8 MHz works just as well and results
> > in picking at least 336 MHz. Also toss in some related w/a numbers.
> >
> > v2: Assume 1 pixel per clock for the purposes of max pixel rate
> >     calculation until DDI clock voltage scaling is handled
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 1241e5891b29..4b8eb6a7d852 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1752,12 +1752,13 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >             crtc_state->has_audio &&
> >             crtc_state->port_clock >= 540000 &&
> >             crtc_state->lane_count == 4) {
> > -               if (IS_CANNONLAKE(dev_priv))
> > -                       pixel_rate = max(316800, pixel_rate);
> > -               else if (IS_GEMINILAKE(dev_priv))
> > +               if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> > +                       /* Display WA #1145: glk,cnl */
> >                         pixel_rate = max(2 * 316800, pixel_rate);
> > -               else
> > +               } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> > +                       /* Display WA #1144: skl,bxt */
> >                         pixel_rate = max(432000, pixel_rate);
> > +               }
> >         }
> >
> >         /* According to BSpec, "The CD clock frequency must be at least twice
> > @@ -1766,7 +1767,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >          * two pixels per clock.
> >          */
> >         if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> > -               if (IS_GEMINILAKE(dev_priv))
> > +               if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> >                         pixel_rate = max(2 * 2 * 96000, pixel_rate);
> >                 else
> >                         pixel_rate = max(2 * 96000, pixel_rate);
> > @@ -1999,7 +2000,14 @@ 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))
> > +       if (INTEL_GEN(dev_priv) >= 10)
> > +               /*
> > +                * FIXME: Allow '2 * max_cdclk_freq'
> > +                * once DDI clock voltage requirements are
> > +                * handled correctly.
> > +                */
> > +               return max_cdclk_freq;
> > +       else if (IS_GEMINILAKE(dev_priv))
> >                 /*
> >                  * FIXME: Limiting to 99% as a temporary workaround. See
> >                  * glk_calc_cdclk() for details.
> 
> Are you sure we don't want this workaround also? With so similar
> display engines I wonder if we would end with similar issues.
> But I'm just asking... because honestly I didn't check that 99%
> workaround closely enough yet...

That workaround seems to be based on empirical evidence and I'd like to
people to get to the bottom of it before it spreads all over the place.

One theory I have is that it might be caused by simply using the wrong
pixel clock when we calculate the pipe's pixel rate. That code currently
assumes that the DPLL can generate a 100% accurate clock which
definitely isn't always the case. IIRC I have code somewhere that
tries to correct that by moving the DPLL computation to happen earlier
than the cdclk computation. That would mean we could then update
adjusted_mode.crtc_clock with the actual clock produced by the DPLL.
I think that patch is stuck in my IVB bifurcation branch and I don't
recall if some of the modeset code reorganization I have there is needed
as well. I guess I'd have to try and extract it and see how much I
need to pull out with it.

> 
> The rest of the patch makes sense for me and works so feel free to use:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> rv-b only for this test for now, but tested-by you could use in all 3
> patches mentioned above...
> 
> 
> > --
> > 2.13.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
Ville Syrjala July 10, 2017, 6:11 p.m. UTC | #4
On Mon, Jul 10, 2017 at 05:34:11PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Mon, 2017-07-10 at 16:02 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Follow the GLK path when computing cdclk and related limits. CNL
> > pipes also produce two pixels per clock, so that's what we should
> > really use. However for the purposes of pixel rate calculations we
> > will assume one pixel per clock to keep the voltage higher, at least
> > until the missing voltage scaling for DDI clocks is implemented.
> > 
> 
> Does the lack of  correct voltage scaling implementation affect only
> intel_compute_max_dotclk()? i.e., allowing a pixel rate of
> 2*max_cdclk_freq? Or does it mean cnl_calc_cdclk() cannot take into
> account pixel_rate <= 2*cdclk_freq for any frequency?
> 
> 
> With this patch, 
> bdw_adjust_min_pipe_pixel_rate() compares pixel_rate to 2*cdclk
> cnl_calc_cdclk() compares pixel_rate to 1*cdclk.
> Isn't that a discrepancy?

Hmm. Yeah. I suppose I should just squash this with patch 2/3. My
original intention for this separate patch was to respect the 2x
limit rather than the 1x limit. But since we couldn't do that I
suppose the justification for this patch has pretty much gone
away, and as you point out, it just leads to a mess.

The combination of patches 1/3+2/3 should still do the right thing
because we no longer use the pixel rate in the audio workarounds.

> 
> 
> > For the HBR2 vs. audio issue the limit should more correctly be 336
> > MHz, but the GLK limit of 316.8 MHz works just as well and results
> > in picking at least 336 MHz. Also toss in some related w/a numbers.
> 
> In this case, _adjust_min_pipe_pixel_rate() will return pixel_rate as
> 633.6 MHz, followed by cnl_calc_cdclk() returning 528 MHz cdclk. But,
> isn't the correct workaround cdclk 336 MHz?
> 
> 
> > 
> > v2: Assume 1 pixel per clock for the purposes of max pixel rate
> >     calculation until DDI clock voltage scaling is handled
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 1241e5891b29..4b8eb6a7d852 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1752,12 +1752,13 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >  	    crtc_state->has_audio &&
> >  	    crtc_state->port_clock >= 540000 &&
> >  	    crtc_state->lane_count == 4) {
> > -		if (IS_CANNONLAKE(dev_priv))
> > -			pixel_rate = max(316800, pixel_rate);
> > -		else if (IS_GEMINILAKE(dev_priv))
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> > +			/* Display WA #1145: glk,cnl */
> >  			pixel_rate = max(2 * 316800, pixel_rate);
> > -		else
> > +		} else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> > +			/* Display WA #1144: skl,bxt */
> >  			pixel_rate = max(432000, pixel_rate);
> > +		}
> >  	}
> >  
> >  	/* According to BSpec, "The CD clock frequency must be at least twice
> > @@ -1766,7 +1767,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> >  	 * two pixels per clock.
> >  	 */
> >  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> > -		if (IS_GEMINILAKE(dev_priv))
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> >  			pixel_rate = max(2 * 2 * 96000, pixel_rate);
> >  		else
> >  			pixel_rate = max(2 * 96000, pixel_rate);
> > @@ -1999,7 +2000,14 @@ 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))
> > +	if (INTEL_GEN(dev_priv) >= 10)
> > +		/*
> > +		 * FIXME: Allow '2 * max_cdclk_freq'
> > +		 * once DDI clock voltage requirements are
> > +		 * handled correctly.
> > +		 */
> > +		return max_cdclk_freq;
> > +	else if (IS_GEMINILAKE(dev_priv))
> >  		/*
> >  		 * FIXME: Limiting to 99% as a temporary workaround. See
> >  		 * glk_calc_cdclk() for details.
Dhinakaran Pandiyan July 10, 2017, 6:36 p.m. UTC | #5
On Mon, 2017-07-10 at 21:11 +0300, Ville Syrjälä wrote:
> On Mon, Jul 10, 2017 at 05:34:11PM +0000, Pandiyan, Dhinakaran wrote:

> > 

> > 

> > 

> > On Mon, 2017-07-10 at 16:02 +0300, ville.syrjala@linux.intel.com wrote:

> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > > 

> > > Follow the GLK path when computing cdclk and related limits. CNL

> > > pipes also produce two pixels per clock, so that's what we should

> > > really use. However for the purposes of pixel rate calculations we

> > > will assume one pixel per clock to keep the voltage higher, at least

> > > until the missing voltage scaling for DDI clocks is implemented.

> > > 

> > 

> > Does the lack of  correct voltage scaling implementation affect only

> > intel_compute_max_dotclk()? i.e., allowing a pixel rate of

> > 2*max_cdclk_freq? Or does it mean cnl_calc_cdclk() cannot take into

> > account pixel_rate <= 2*cdclk_freq for any frequency?

> > 

> > 

> > With this patch, 

> > bdw_adjust_min_pipe_pixel_rate() compares pixel_rate to 2*cdclk

> > cnl_calc_cdclk() compares pixel_rate to 1*cdclk.

> > Isn't that a discrepancy?

> 

> Hmm. Yeah. I suppose I should just squash this with patch 2/3. My

> original intention for this separate patch was to respect the 2x

> limit rather than the 1x limit. But since we couldn't do that I

> suppose the justification for this patch has pretty much gone

> away, and as you point out, it just leads to a mess.

> 

> The combination of patches 1/3+2/3 should still do the right thing

> because we no longer use the pixel rate in the audio workarounds.

> 


I just took a look at 2/3, squashing does solve the problem. I'll review
the combined patch when you send it.

-DK 

> > 

> > 

> > > For the HBR2 vs. audio issue the limit should more correctly be 336

> > > MHz, but the GLK limit of 316.8 MHz works just as well and results

> > > in picking at least 336 MHz. Also toss in some related w/a numbers.

> > 

> > In this case, _adjust_min_pipe_pixel_rate() will return pixel_rate as

> > 633.6 MHz, followed by cnl_calc_cdclk() returning 528 MHz cdclk. But,

> > isn't the correct workaround cdclk 336 MHz?

> > 

> > 

> > > 

> > > v2: Assume 1 pixel per clock for the purposes of max pixel rate

> > >     calculation until DDI clock voltage scaling is handled

> > > 

> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > > ---

> > >  drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++------

> > >  1 file changed, 14 insertions(+), 6 deletions(-)

> > > 

> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c

> > > index 1241e5891b29..4b8eb6a7d852 100644

> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c

> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c

> > > @@ -1752,12 +1752,13 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,

> > >  	    crtc_state->has_audio &&

> > >  	    crtc_state->port_clock >= 540000 &&

> > >  	    crtc_state->lane_count == 4) {

> > > -		if (IS_CANNONLAKE(dev_priv))

> > > -			pixel_rate = max(316800, pixel_rate);

> > > -		else if (IS_GEMINILAKE(dev_priv))

> > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {

> > > +			/* Display WA #1145: glk,cnl */

> > >  			pixel_rate = max(2 * 316800, pixel_rate);

> > > -		else

> > > +		} else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {

> > > +			/* Display WA #1144: skl,bxt */

> > >  			pixel_rate = max(432000, pixel_rate);

> > > +		}

> > >  	}

> > >  

> > >  	/* According to BSpec, "The CD clock frequency must be at least twice

> > > @@ -1766,7 +1767,7 @@ static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,

> > >  	 * two pixels per clock.

> > >  	 */

> > >  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {

> > > -		if (IS_GEMINILAKE(dev_priv))

> > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))

> > >  			pixel_rate = max(2 * 2 * 96000, pixel_rate);

> > >  		else

> > >  			pixel_rate = max(2 * 96000, pixel_rate);

> > > @@ -1999,7 +2000,14 @@ 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))

> > > +	if (INTEL_GEN(dev_priv) >= 10)

> > > +		/*

> > > +		 * FIXME: Allow '2 * max_cdclk_freq'

> > > +		 * once DDI clock voltage requirements are

> > > +		 * handled correctly.

> > > +		 */

> > > +		return max_cdclk_freq;

> > > +	else if (IS_GEMINILAKE(dev_priv))

> > >  		/*

> > >  		 * FIXME: Limiting to 99% as a temporary workaround. See

> > >  		 * glk_calc_cdclk() for details.

>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 1241e5891b29..4b8eb6a7d852 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1752,12 +1752,13 @@  static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	    crtc_state->has_audio &&
 	    crtc_state->port_clock >= 540000 &&
 	    crtc_state->lane_count == 4) {
-		if (IS_CANNONLAKE(dev_priv))
-			pixel_rate = max(316800, pixel_rate);
-		else if (IS_GEMINILAKE(dev_priv))
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
+			/* Display WA #1145: glk,cnl */
 			pixel_rate = max(2 * 316800, pixel_rate);
-		else
+		} else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
+			/* Display WA #1144: skl,bxt */
 			pixel_rate = max(432000, pixel_rate);
+		}
 	}
 
 	/* According to BSpec, "The CD clock frequency must be at least twice
@@ -1766,7 +1767,7 @@  static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
 	 * two pixels per clock.
 	 */
 	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
-		if (IS_GEMINILAKE(dev_priv))
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
 			pixel_rate = max(2 * 2 * 96000, pixel_rate);
 		else
 			pixel_rate = max(2 * 96000, pixel_rate);
@@ -1999,7 +2000,14 @@  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))
+	if (INTEL_GEN(dev_priv) >= 10)
+		/*
+		 * FIXME: Allow '2 * max_cdclk_freq'
+		 * once DDI clock voltage requirements are
+		 * handled correctly.
+		 */
+		return max_cdclk_freq;
+	else if (IS_GEMINILAKE(dev_priv))
 		/*
 		 * FIXME: Limiting to 99% as a temporary workaround. See
 		 * glk_calc_cdclk() for details.