* [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK
2017-03-01 2:57 [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio Dhinakaran Pandiyan
@ 2017-03-01 2:57 ` Dhinakaran Pandiyan
2017-03-03 12:23 ` Ander Conselvan De Oliveira
2017-03-01 4:22 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/glk: Apply cdclk workaround for DP audio Patchwork
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2017-03-01 2:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira, Dhinakaran Pandiyan
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);
}
return min_cdclk;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK
2017-03-01 2:57 ` [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK Dhinakaran Pandiyan
@ 2017-03-03 12:23 ` Ander Conselvan De Oliveira
2017-03-04 0:08 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 8+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-03-03 12:23 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx, Zanoni, Paulo R
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));
?
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?
Ander
> }
>
> return min_cdclk;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK
2017-03-03 12:23 ` Ander Conselvan De Oliveira
@ 2017-03-04 0:08 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 8+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-04 0:08 UTC (permalink / raw)
To: conselvan2@gmail.com; +Cc: intel-gfx@lists.freedesktop.org, Zanoni, Paulo R
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/glk: Apply cdclk workaround for DP audio
2017-03-01 2:57 [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio Dhinakaran Pandiyan
2017-03-01 2:57 ` [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK Dhinakaran Pandiyan
@ 2017-03-01 4:22 ` Patchwork
2017-03-03 12:14 ` [PATCH 1/2] " Ander Conselvan De Oliveira
2017-03-03 17:55 ` Paulo Zanoni
3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2017-03-01 4:22 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915/glk: Apply cdclk workaround for DP audio
URL : https://patchwork.freedesktop.org/series/20430/
State : warning
== Summary ==
Series 20430v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20430/revisions/1/mbox/
Test gem_exec_parallel:
Subgroup basic:
pass -> DMESG-WARN (fi-skl-6700k)
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19
fi-bxt-t5700 total:108 pass:95 dwarn:0 dfail:0 fail:0 skip:12
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18
fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17
fi-skl-6700k total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29
5d37006b578e38562382215e8782cfced9c992ce drm-tip: 2017y-02m-28d-16h-27m-13s UTC integration manifest
bc61fcd drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK
122bad9 drm/i915/glk: Apply cdclk workaround for DP audio
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4011/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio
2017-03-01 2:57 [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio Dhinakaran Pandiyan
2017-03-01 2:57 ` [PATCH 2/2] drm/i915: Implement BXT and GLK cdclk restriction based on Azalia BCLK Dhinakaran Pandiyan
2017-03-01 4:22 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/glk: Apply cdclk workaround for DP audio Patchwork
@ 2017-03-03 12:14 ` Ander Conselvan De Oliveira
2017-03-03 17:55 ` Paulo Zanoni
3 siblings, 0 replies; 8+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-03-03 12:14 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx
On Tue, 2017-02-28 at 18:57 -0800, Dhinakaran Pandiyan wrote:
> Implement GLK cdclk restriction for DP audio, similar to what's implemented
> for BDW and other GEN9 platforms. The cdclk restriction has been
> refactored out of max. pixel clock computation as the 1:1 relationship
> between pixel clock and cdclk frequency does not hold for GLK.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++--------------
> 1 file changed, 52 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..8fc0f72 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1069,11 +1069,11 @@ static int bxt_calc_cdclk(int max_pixclk)
> return 144000;
> }
>
> -static int glk_calc_cdclk(int max_pixclk)
> +static int glk_calc_cdclk(int max_pixclk, int min_cdclk)
> {
> - if (max_pixclk > 2 * 158400)
> + if (max_pixclk > 2 * 158400 || min_cdclk > 158400)
> return 316800;
> - else if (max_pixclk > 2 * 79200)
> + else if (max_pixclk > 2 * 79200 || min_cdclk > 79200)
> return 158400;
> else
> return 79200;
> @@ -1367,7 +1367,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv)
> * Need to make this change after VBT has changes for BXT.
> */
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk_state.cdclk = glk_calc_cdclk(0);
> + cdclk_state.cdclk = glk_calc_cdclk(0, 0);
> cdclk_state.vco = glk_de_pll_vco(dev_priv, cdclk_state.cdclk);
> } else {
> cdclk_state.cdclk = bxt_calc_cdclk(0);
> @@ -1432,28 +1432,37 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> }
>
> -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> - int pixel_rate)
> +static int intel_min_cdclk(struct drm_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv =
> - to_i915(crtc_state->base.crtc->dev);
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> + int min_cdclk = 0;
>
> - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + for_each_crtc_in_state(state, crtc, cstate, i) {
> + struct intel_crtc_state *crtc_state;
>
> - /* BSpec says "Do not use DisplayPort with CDCLK less than
> - * 432 MHz, audio enabled, port width x4, and link rate
> - * HBR2 (5.4 GHz), or else there may be audio corruption or
> - * screen corruption."
> - */
> - if (intel_crtc_has_dp_encoder(crtc_state) &&
> - crtc_state->has_audio &&
> - crtc_state->port_clock >= 540000 &&
> - crtc_state->lane_count == 4)
> - pixel_rate = max(432000, pixel_rate);
> + crtc_state = to_intel_crtc_state(cstate);
> +
> + /* According to BSpec, "Do not use DisplayPort with CDCLK less
> + * than 432 MHz, audio enabled, port width x4, and link rate
> + * HBR2 (5.4 GHz), or else there may be audio corruption or
> + * screen corruption." for BDW and GEN9. The cdclk restriction
> + * 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))
> + min_cdclk = 316800;
> + else if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> + min_cdclk = 432000;
> + }
> + }
>
> - return pixel_rate;
> + return min_cdclk;
> }
>
> /* compute the max rate for new configuration */
> @@ -1481,10 +1490,9 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>
> pixel_rate = crtc_state->pixel_rate;
>
> - if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> - pixel_rate =
> - bdw_adjust_min_pipe_pixel_rate(crtc_state,
> - pixel_rate);
> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> + pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>
> intel_state->min_pixclk[i] = pixel_rate;
> }
> @@ -1531,13 +1539,17 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk;
>
> /*
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = bdw_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bdw_calc_cdclk(min_cdclk);
> + else
> + cdclk = bdw_calc_cdclk(max_pixclk);
Looks like intel_min_cdclk() returns a valid cdclk for the platform, so wouldn't
cdclk = *_calc_cdclk(max_pixclk);
if (cdclk < min_cdclk)
cdclk = min_cdclk;
be simpler?
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1564,6 +1576,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> const int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> vco = intel_state->cdclk.logical.vco;
> @@ -1574,7 +1587,10 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = skl_calc_cdclk(max_pixclk, vco);
> + if (min_cdclk > max_pixclk)
> + cdclk = skl_calc_cdclk(min_cdclk, vco);
> + else
> + cdclk = skl_calc_cdclk(max_pixclk, vco);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1604,13 +1620,18 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> int max_pixclk = intel_max_pixel_rate(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(max_pixclk);
> + cdclk = glk_calc_cdclk(max_pixclk, min_cdclk);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> - cdclk = bxt_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bxt_calc_cdclk(min_cdclk);
> + else
> + cdclk = bxt_calc_cdclk(max_pixclk);
> +
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
With the idea above, this would become
if (IS_GEMINILAKE(dev_priv))
cdclk = glk_calc_cdclk(max_pixclk);
else
cdclk = bxt_calc_cdclk(max_pixclk);
if (cdclk < min_cdclk)
cdclk = min_cdclk;
if (IS_GEMINILAKE(dev_priv))
vco = glk_de_pll_vco(dev_priv, cdclk);
else
vco = bxt_de_pll_vco(dev_priv, cdclk);
Not great, but maybe still better than changing the signature of
glk_calc_cdclk()?
Also, this conflicts with Paulo's cdclk clean up [1].
[1] https://patchwork.freedesktop.org/series/19974/
Ander
>
> @@ -1625,7 +1646,7 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> if (!intel_state->active_crtcs) {
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(0);
> + cdclk = glk_calc_cdclk(0, 0);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> cdclk = bxt_calc_cdclk(0);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio
2017-03-01 2:57 [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio Dhinakaran Pandiyan
` (2 preceding siblings ...)
2017-03-03 12:14 ` [PATCH 1/2] " Ander Conselvan De Oliveira
@ 2017-03-03 17:55 ` Paulo Zanoni
2017-03-03 23:39 ` Pandiyan, Dhinakaran
3 siblings, 1 reply; 8+ messages in thread
From: Paulo Zanoni @ 2017-03-03 17:55 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx; +Cc: Ander Conselvan de Oliveira
Em Ter, 2017-02-28 às 18:57 -0800, Dhinakaran Pandiyan escreveu:
> Implement GLK cdclk restriction for DP audio, similar to what's
> implemented
> for BDW and other GEN9 platforms. The cdclk restriction has been
> refactored out of max. pixel clock computation as the 1:1
> relationship
> between pixel clock and cdclk frequency does not hold for GLK.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++--
> ------------
> 1 file changed, 52 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..8fc0f72 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1069,11 +1069,11 @@ static int bxt_calc_cdclk(int max_pixclk)
> return 144000;
> }
>
> -static int glk_calc_cdclk(int max_pixclk)
> +static int glk_calc_cdclk(int max_pixclk, int min_cdclk)
> {
> - if (max_pixclk > 2 * 158400)
> + if (max_pixclk > 2 * 158400 || min_cdclk > 158400)
> return 316800;
> - else if (max_pixclk > 2 * 79200)
> + else if (max_pixclk > 2 * 79200 || min_cdclk > 79200)
> return 158400;
> else
> return 79200;
> @@ -1367,7 +1367,7 @@ void bxt_init_cdclk(struct drm_i915_private
> *dev_priv)
> * Need to make this change after VBT has changes for BXT.
> */
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk_state.cdclk = glk_calc_cdclk(0);
> + cdclk_state.cdclk = glk_calc_cdclk(0, 0);
> cdclk_state.vco = glk_de_pll_vco(dev_priv,
> cdclk_state.cdclk);
> } else {
> cdclk_state.cdclk = bxt_calc_cdclk(0);
> @@ -1432,28 +1432,37 @@ void intel_set_cdclk(struct drm_i915_private
> *dev_priv,
> dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> }
>
> -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> *crtc_state,
> - int pixel_rate)
> +static int intel_min_cdclk(struct drm_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv =
> - to_i915(crtc_state->base.crtc->dev);
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> + int min_cdclk = 0;
>
> - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + for_each_crtc_in_state(state, crtc, cstate, i) {
> + struct intel_crtc_state *crtc_state;
Here we're just looking at the CRTCs in the state. Notice that
max_pixel_rate caches the values for all the CRTCs so we can account
for all of them instead of just the ones that are in the current atomic
commit.
What if some other CRTC not in the current state raised the minimum
clock to 432/316? Don't we end up risking not noticing it and going
with a lower clock here? Imagine two very-low-res monitors with audio
enabled. First we do the modeset on the audio monitor, then we enable
the other monitor. Won't the second modeset end up wrongly reducing the
clock below 432/316?
In other words: why doesn't min_cdclk need to do the same caching
scheme that max_cdclk needs?
>
> - /* BSpec says "Do not use DisplayPort with CDCLK less than
> - * 432 MHz, audio enabled, port width x4, and link rate
> - * HBR2 (5.4 GHz), or else there may be audio corruption or
> - * screen corruption."
> - */
> - if (intel_crtc_has_dp_encoder(crtc_state) &&
> - crtc_state->has_audio &&
> - crtc_state->port_clock >= 540000 &&
> - crtc_state->lane_count == 4)
> - pixel_rate = max(432000, pixel_rate);
> + crtc_state = to_intel_crtc_state(cstate);
Can you please clarify why it's not possible to simply add a new check
for GLK in the old function? That would have been simpler.
If we still want to go with this approach, I'd suggest splitting your
commit in two separate commits: one reworking the current code (and
addressing the issue I pointed above), and another adding the GLK
stuff.
> +
> + /* According to BSpec, "Do not use DisplayPort with
> CDCLK less
> + * than 432 MHz, audio enabled, port width x4, and
> link rate
> + * HBR2 (5.4 GHz), or else there may be audio
> corruption or
> + * screen corruption." for BDW and GEN9. The cdclk
> restriction
> + * 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))
> + min_cdclk = 316800;
> + else if (IS_BROADWELL(dev_priv) ||
> IS_GEN9(dev_priv))
> + min_cdclk = 432000;
> + }
> + }
>
> - return pixel_rate;
> + return min_cdclk;
> }
>
> /* compute the max rate for new configuration */
> @@ -1481,10 +1490,9 @@ static int intel_max_pixel_rate(struct
> drm_atomic_state *state)
>
> pixel_rate = crtc_state->pixel_rate;
>
> - if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> - pixel_rate =
> - bdw_adjust_min_pipe_pixel_rate(crtc_
> state,
> - pixel
> _rate);
> + /* pixel rate mustn't exceed 95% of cdclk with IPS
> on BDW */
> + if (IS_BROADWELL(dev_priv) && crtc_state-
> >ips_enabled)
> + pixel_rate = DIV_ROUND_UP(pixel_rate * 100,
> 95);
>
> intel_state->min_pixclk[i] = pixel_rate;
> }
> @@ -1531,13 +1539,17 @@ static int bdw_modeset_calc_cdclk(struct
> drm_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk;
>
> /*
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = bdw_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bdw_calc_cdclk(min_cdclk);
> + else
> + cdclk = bdw_calc_cdclk(max_pixclk);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> (%d kHz)\n",
> @@ -1564,6 +1576,7 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state)
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> const int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> vco = intel_state->cdclk.logical.vco;
> @@ -1574,7 +1587,10 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state)
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = skl_calc_cdclk(max_pixclk, vco);
> + if (min_cdclk > max_pixclk)
> + cdclk = skl_calc_cdclk(min_cdclk, vco);
> + else
> + cdclk = skl_calc_cdclk(max_pixclk, vco);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> (%d kHz)\n",
> @@ -1604,13 +1620,18 @@ static int bxt_modeset_calc_cdclk(struct
> drm_atomic_state *state)
> int max_pixclk = intel_max_pixel_rate(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> + int min_cdclk = intel_min_cdclk(state);
> int cdclk, vco;
>
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(max_pixclk);
> + cdclk = glk_calc_cdclk(max_pixclk, min_cdclk);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> - cdclk = bxt_calc_cdclk(max_pixclk);
> + if (min_cdclk > max_pixclk)
> + cdclk = bxt_calc_cdclk(min_cdclk);
> + else
> + cdclk = bxt_calc_cdclk(max_pixclk);
> +
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
>
> @@ -1625,7 +1646,7 @@ static int bxt_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>
> if (!intel_state->active_crtcs) {
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(0);
> + cdclk = glk_calc_cdclk(0, 0);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> cdclk = bxt_calc_cdclk(0);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio
2017-03-03 17:55 ` Paulo Zanoni
@ 2017-03-03 23:39 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 8+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-03 23:39 UTC (permalink / raw)
To: Zanoni, Paulo R
Cc: Conselvan De Oliveira, Ander, intel-gfx@lists.freedesktop.org
On Fri, 2017-03-03 at 14:55 -0300, Paulo Zanoni wrote:
> Em Ter, 2017-02-28 às 18:57 -0800, Dhinakaran Pandiyan escreveu:
> > Implement GLK cdclk restriction for DP audio, similar to what's
> > implemented
> > for BDW and other GEN9 platforms. The cdclk restriction has been
> > refactored out of max. pixel clock computation as the 1:1
> > relationship
> > between pixel clock and cdclk frequency does not hold for GLK.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++--
> > ------------
> > 1 file changed, 52 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index d643c0c..8fc0f72 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1069,11 +1069,11 @@ static int bxt_calc_cdclk(int max_pixclk)
> > return 144000;
> > }
> >
> > -static int glk_calc_cdclk(int max_pixclk)
> > +static int glk_calc_cdclk(int max_pixclk, int min_cdclk)
> > {
> > - if (max_pixclk > 2 * 158400)
> > + if (max_pixclk > 2 * 158400 || min_cdclk > 158400)
> > return 316800;
> > - else if (max_pixclk > 2 * 79200)
> > + else if (max_pixclk > 2 * 79200 || min_cdclk > 79200)
> > return 158400;
> > else
> > return 79200;
> > @@ -1367,7 +1367,7 @@ void bxt_init_cdclk(struct drm_i915_private
> > *dev_priv)
> > * Need to make this change after VBT has changes for BXT.
> > */
> > if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk_state.cdclk = glk_calc_cdclk(0);
> > + cdclk_state.cdclk = glk_calc_cdclk(0, 0);
> > cdclk_state.vco = glk_de_pll_vco(dev_priv,
> > cdclk_state.cdclk);
> > } else {
> > cdclk_state.cdclk = bxt_calc_cdclk(0);
> > @@ -1432,28 +1432,37 @@ void intel_set_cdclk(struct drm_i915_private
> > *dev_priv,
> > dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > }
> >
> > -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> > *crtc_state,
> > - int pixel_rate)
> > +static int intel_min_cdclk(struct drm_atomic_state *state)
> > {
> > - struct drm_i915_private *dev_priv =
> > - to_i915(crtc_state->base.crtc->dev);
> > + struct drm_i915_private *dev_priv = to_i915(state->dev);
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *cstate;
> > + int i;
> > + int min_cdclk = 0;
> >
> > - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> > + for_each_crtc_in_state(state, crtc, cstate, i) {
> > + struct intel_crtc_state *crtc_state;
>
> Here we're just looking at the CRTCs in the state. Notice that
> max_pixel_rate caches the values for all the CRTCs so we can account
> for all of them instead of just the ones that are in the current atomic
> commit.
>
> What if some other CRTC not in the current state raised the minimum
> clock to 432/316? Don't we end up risking not noticing it and going
> with a lower clock here? Imagine two very-low-res monitors with audio
> enabled. First we do the modeset on the audio monitor, then we enable
> the other monitor. Won't the second modeset end up wrongly reducing the
> clock below 432/316?
>
> In other words: why doesn't min_cdclk need to do the same caching
> scheme that max_cdclk needs?
>
You are right, I wonder if intel_atomic_state.cdclk would be a good
place to cache that.
> >
> > - /* BSpec says "Do not use DisplayPort with CDCLK less than
> > - * 432 MHz, audio enabled, port width x4, and link rate
> > - * HBR2 (5.4 GHz), or else there may be audio corruption or
> > - * screen corruption."
> > - */
> > - if (intel_crtc_has_dp_encoder(crtc_state) &&
> > - crtc_state->has_audio &&
> > - crtc_state->port_clock >= 540000 &&
> > - crtc_state->lane_count == 4)
> > - pixel_rate = max(432000, pixel_rate);
> > + crtc_state = to_intel_crtc_state(cstate);
>
> Can you please clarify why it's not possible to simply add a new check
> for GLK in the old function? That would have been simpler.
>
The old function, bdw_adjust_min_pipe_pixel_rate(), assumes that cdclk
is same as the pixel rate and returns min. cdclk to
intel_max_pixel_rate() when audio workarounds have to applied. This
cdclk value is passed on to glk_calc_cdclk(), which needs pixel rate as
input and is compared against 2x cdclk to arrive at a valid cdclk. So,
passing in cdclk to glk_calc_cdclk(), instead of pixel rate is a bug.
The new function I am adding separates the pixel rate computation and
min cdclk restriction.
The other option I can think of is to change intel_max_pixel_rate() to
return both the min cdclk and max pixel rate.
-DK
> If we still want to go with this approach, I'd suggest splitting your
> commit in two separate commits: one reworking the current code (and
> addressing the issue I pointed above), and another adding the GLK
> stuff.
>
>
> > +
> > + /* According to BSpec, "Do not use DisplayPort with
> > CDCLK less
> > + * than 432 MHz, audio enabled, port width x4, and
> > link rate
> > + * HBR2 (5.4 GHz), or else there may be audio
> > corruption or
> > + * screen corruption." for BDW and GEN9. The cdclk
> > restriction
> > + * 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))
> > + min_cdclk = 316800;
> > + else if (IS_BROADWELL(dev_priv) ||
> > IS_GEN9(dev_priv))
> > + min_cdclk = 432000;
> > + }
> > + }
> >
> > - return pixel_rate;
> > + return min_cdclk;
> > }
> >
> > /* compute the max rate for new configuration */
> > @@ -1481,10 +1490,9 @@ static int intel_max_pixel_rate(struct
> > drm_atomic_state *state)
> >
> > pixel_rate = crtc_state->pixel_rate;
> >
> > - if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> > - pixel_rate =
> > - bdw_adjust_min_pipe_pixel_rate(crtc_
> > state,
> > - pixel
> > _rate);
> > + /* pixel rate mustn't exceed 95% of cdclk with IPS
> > on BDW */
> > + if (IS_BROADWELL(dev_priv) && crtc_state-
> > >ips_enabled)
> > + pixel_rate = DIV_ROUND_UP(pixel_rate * 100,
> > 95);
> >
> > intel_state->min_pixclk[i] = pixel_rate;
> > }
> > @@ -1531,13 +1539,17 @@ static int bdw_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_min_cdclk(state);
> > int cdclk;
> >
> > /*
> > * FIXME should also account for plane ratio
> > * once 64bpp pixel formats are supported.
> > */
> > - cdclk = bdw_calc_cdclk(max_pixclk);
> > + if (min_cdclk > max_pixclk)
> > + cdclk = bdw_calc_cdclk(min_cdclk);
> > + else
> > + cdclk = bdw_calc_cdclk(max_pixclk);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> > @@ -1564,6 +1576,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > const int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_min_cdclk(state);
> > int cdclk, vco;
> >
> > vco = intel_state->cdclk.logical.vco;
> > @@ -1574,7 +1587,10 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > * FIXME should also account for plane ratio
> > * once 64bpp pixel formats are supported.
> > */
> > - cdclk = skl_calc_cdclk(max_pixclk, vco);
> > + if (min_cdclk > max_pixclk)
> > + cdclk = skl_calc_cdclk(min_cdclk, vco);
> > + else
> > + cdclk = skl_calc_cdclk(max_pixclk, vco);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> > @@ -1604,13 +1620,18 @@ static int bxt_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > int max_pixclk = intel_max_pixel_rate(state);
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > + int min_cdclk = intel_min_cdclk(state);
> > int cdclk, vco;
> >
> > if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk = glk_calc_cdclk(max_pixclk);
> > + cdclk = glk_calc_cdclk(max_pixclk, min_cdclk);
> > vco = glk_de_pll_vco(dev_priv, cdclk);
> > } else {
> > - cdclk = bxt_calc_cdclk(max_pixclk);
> > + if (min_cdclk > max_pixclk)
> > + cdclk = bxt_calc_cdclk(min_cdclk);
> > + else
> > + cdclk = bxt_calc_cdclk(max_pixclk);
> > +
> > vco = bxt_de_pll_vco(dev_priv, cdclk);
> > }
> >
> > @@ -1625,7 +1646,7 @@ static int bxt_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >
> > if (!intel_state->active_crtcs) {
> > if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk = glk_calc_cdclk(0);
> > + cdclk = glk_calc_cdclk(0, 0);
> > vco = glk_de_pll_vco(dev_priv, cdclk);
> > } else {
> > cdclk = bxt_calc_cdclk(0);
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread