| Message ID | 20200630112609.9998-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915: Clamp min_cdclk to max_cdclk_freq to unblock 8K | expand |
On Tue, Jun 30, 2020 at 02:26:09PM +0300, Stanislav Lisovskiy wrote: > We still need "Bump up CDCLK" workaround otherwise getting > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > in 8K case would require CDCLK to be around 1 Ghz which is not > possible. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 45f7f33d1144..01a5bc6b08c4 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > * Explicitly stating here that this seems to be currently > * rather a Hack, than final solution. > */ > - if (IS_TIGERLAKE(dev_priv)) > + if (IS_TIGERLAKE(dev_priv)) { > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > + /* > + * Clamp to max_cdclk_freq in order not to break an 8K, > + * but still leave W/A at place. > + */ > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > + > + /* > + * max_cdclk_freq check obviously not needed - just return. > + */ > + return min_cdclk; Pointless return. But I think we should actually keep the max_cdclk check. Something like: min_cdclk = max(min_cdclk, min(max_cdclk, pixel_rate)); Also what's with the (int) casts? There is min_t() if you actually need casts. But not sure why we need them though. > + } > + > if (min_cdclk > dev_priv->max_cdclk_freq) { > drm_dbg_kms(&dev_priv->drm, > "required cdclk (%d kHz) exceeds max (%d kHz)\n", > -- > 2.24.1.485.gad05a3d8e5
On Tue, Jun 30, 2020 at 07:29:09PM +0300, Ville Syrjälä wrote: > On Tue, Jun 30, 2020 at 02:26:09PM +0300, Stanislav Lisovskiy wrote: > > We still need "Bump up CDCLK" workaround otherwise getting > > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > > in 8K case would require CDCLK to be around 1 Ghz which is not > > possible. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 45f7f33d1144..01a5bc6b08c4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > * Explicitly stating here that this seems to be currently > > * rather a Hack, than final solution. > > */ > > - if (IS_TIGERLAKE(dev_priv)) > > + if (IS_TIGERLAKE(dev_priv)) { > > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > > > + /* > > + * Clamp to max_cdclk_freq in order not to break an 8K, > > + * but still leave W/A at place. > > + */ > > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > > + > > + /* > > + * max_cdclk_freq check obviously not needed - just return. > > + */ > > + return min_cdclk; > > Pointless return. But I think we should actually keep the max_cdclk > check. Something like: > > min_cdclk = max(min_cdclk, > min(max_cdclk, pixel_rate)); > > Also what's with the (int) casts? There is min_t() if you > actually need casts. But not sure why we need them though. Yes this logic suggested by Ville bumps up the min cdclock to either the pixel rate or max cdclk freq whichever is the min so we dont run into cdcclk not sufficient error for 8K since the 8K (3840 x 4320 1 tile) would need ~ 1066Mhz pixel rate. Changing to this logic, you can count my r-b Manasi > > > + } > > + > > if (min_cdclk > dev_priv->max_cdclk_freq) { > > drm_dbg_kms(&dev_priv->drm, > > "required cdclk (%d kHz) exceeds max (%d kHz)\n", > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Tue, Jun 30, 2020 at 07:29:09PM +0300, Ville Syrjälä wrote: > On Tue, Jun 30, 2020 at 02:26:09PM +0300, Stanislav Lisovskiy wrote: > > We still need "Bump up CDCLK" workaround otherwise getting > > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > > in 8K case would require CDCLK to be around 1 Ghz which is not > > possible. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 45f7f33d1144..01a5bc6b08c4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > * Explicitly stating here that this seems to be currently > > * rather a Hack, than final solution. > > */ > > - if (IS_TIGERLAKE(dev_priv)) > > + if (IS_TIGERLAKE(dev_priv)) { > > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > > > + /* > > + * Clamp to max_cdclk_freq in order not to break an 8K, > > + * but still leave W/A at place. > > + */ > > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > > + > > + /* > > + * max_cdclk_freq check obviously not needed - just return. > > + */ > > + return min_cdclk; > > Pointless return. But I think we should actually keep the max_cdclk > check. Something like: > > min_cdclk = max(min_cdclk, > min(max_cdclk, pixel_rate)); > > Also what's with the (int) casts? There is min_t() if you > actually need casts. But not sure why we need them though. Don't like the casts either. Aware of min_t. Just remember that smth prevented me from using it in initial patch. Will check once again to figure out why I did so last time. Regarding the check we actually you are proposing to do the same thing as initially I check min_cdclk = max(min_cdclk, pixel_rate) and then it gets to min_cdclk = min(min_cdclk, max_cdclk_freq). Just do it kind of in 2 steps in order not to change the original Bump CDCLK patch. I.e min_cdclk still has to be more or equal than pixel rate, but always less than max_cdclk_freq. Stan > > > + } > > + > > if (min_cdclk > dev_priv->max_cdclk_freq) { > > drm_dbg_kms(&dev_priv->drm, > > "required cdclk (%d kHz) exceeds max (%d kHz)\n", > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
Op 30-06-2020 om 13:26 schreef Stanislav Lisovskiy: > We still need "Bump up CDCLK" workaround otherwise getting > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > in 8K case would require CDCLK to be around 1 Ghz which is not > possible. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 45f7f33d1144..01a5bc6b08c4 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > * Explicitly stating here that this seems to be currently > * rather a Hack, than final solution. > */ > - if (IS_TIGERLAKE(dev_priv)) > + if (IS_TIGERLAKE(dev_priv)) { > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > + /* > + * Clamp to max_cdclk_freq in order not to break an 8K, > + * but still leave W/A at place. > + */ > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > + > + /* > + * max_cdclk_freq check obviously not needed - just return. > + */ > + return min_cdclk; > + } > + > if (min_cdclk > dev_priv->max_cdclk_freq) { > drm_dbg_kms(&dev_priv->drm, > "required cdclk (%d kHz) exceeds max (%d kHz)\n", Wouldn't you just have to halve pixel_rate if bigjoiner flag is set?
On Wed, Jul 01, 2020 at 02:20:28PM +0200, Maarten Lankhorst wrote: > Op 30-06-2020 om 13:26 schreef Stanislav Lisovskiy: > > We still need "Bump up CDCLK" workaround otherwise getting > > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > > in 8K case would require CDCLK to be around 1 Ghz which is not > > possible. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 45f7f33d1144..01a5bc6b08c4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > * Explicitly stating here that this seems to be currently > > * rather a Hack, than final solution. > > */ > > - if (IS_TIGERLAKE(dev_priv)) > > + if (IS_TIGERLAKE(dev_priv)) { > > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > > > + /* > > + * Clamp to max_cdclk_freq in order not to break an 8K, > > + * but still leave W/A at place. > > + */ > > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > > + > > + /* > > + * max_cdclk_freq check obviously not needed - just return. > > + */ > > + return min_cdclk; > > + } > > + > > if (min_cdclk > dev_priv->max_cdclk_freq) { > > drm_dbg_kms(&dev_priv->drm, > > "required cdclk (%d kHz) exceeds max (%d kHz)\n", > > Wouldn't you just have to halve pixel_rate if bigjoiner flag is set? We dont have big joiner patches pulled in yet, this is just for the 2p2p configuration Manasi >
On Wed, Jul 01, 2020 at 11:44:04AM -0700, Manasi Navare wrote: > On Wed, Jul 01, 2020 at 02:20:28PM +0200, Maarten Lankhorst wrote: > > Op 30-06-2020 om 13:26 schreef Stanislav Lisovskiy: > > > We still need "Bump up CDCLK" workaround otherwise getting > > > underruns - however currently it blocks 8K as CDCLK = Pixel rate, > > > in 8K case would require CDCLK to be around 1 Ghz which is not > > > possible. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > index 45f7f33d1144..01a5bc6b08c4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > > * Explicitly stating here that this seems to be currently > > > * rather a Hack, than final solution. > > > */ > > > - if (IS_TIGERLAKE(dev_priv)) > > > + if (IS_TIGERLAKE(dev_priv)) { > > > min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); > > > > > > + /* > > > + * Clamp to max_cdclk_freq in order not to break an 8K, > > > + * but still leave W/A at place. > > > + */ > > > + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); > > > + > > > + /* > > > + * max_cdclk_freq check obviously not needed - just return. > > > + */ > > > + return min_cdclk; > > > + } > > > + > > > if (min_cdclk > dev_priv->max_cdclk_freq) { > > > drm_dbg_kms(&dev_priv->drm, > > > "required cdclk (%d kHz) exceeds max (%d kHz)\n", > > > > Wouldn't you just have to halve pixel_rate if bigjoiner flag is set? > > We dont have big joiner patches pulled in yet, this is just for the 2p2p configuration > > Manasi Also it would make more sense if we wanted this to stay here, however I still want to believe that some day(R) we figure out proper solution. Otherwise yep, we would need some logic to check if the pixel rate should be divided and etc.. Stan > > >
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 45f7f33d1144..01a5bc6b08c4 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2080,9 +2080,21 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) * Explicitly stating here that this seems to be currently * rather a Hack, than final solution. */ - if (IS_TIGERLAKE(dev_priv)) + if (IS_TIGERLAKE(dev_priv)) { min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate); + /* + * Clamp to max_cdclk_freq in order not to break an 8K, + * but still leave W/A at place. + */ + min_cdclk = min(min_cdclk, (int)dev_priv->max_cdclk_freq); + + /* + * max_cdclk_freq check obviously not needed - just return. + */ + return min_cdclk; + } + if (min_cdclk > dev_priv->max_cdclk_freq) { drm_dbg_kms(&dev_priv->drm, "required cdclk (%d kHz) exceeds max (%d kHz)\n",
We still need "Bump up CDCLK" workaround otherwise getting underruns - however currently it blocks 8K as CDCLK = Pixel rate, in 8K case would require CDCLK to be around 1 Ghz which is not possible. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)