| Message ID | 20210908003944.2972024-4-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915/display: split and constify vtable | expand |
On Wed, 08 Sep 2021, Dave Airlie <airlied@gmail.com> wrote: > From: Dave Airlie <airlied@redhat.com> > > A vague goal is to have the vfunc table be the api between > wm and display, not having direction function calls cross > the boundary. > > This aligns the legacy update_wm with the newer vfuncs. > > The comment probably needs to live somewhere else, it seems > like it should live in the pm side though not the display side, > but I brought it along for the ride. > --- > drivers/gpu/drm/i915/display/intel_display.c | 40 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 39 ------------------- > drivers/gpu/drm/i915/intel_pm.h | 1 - > 3 files changed, 40 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index d95283bf2631..b495371c1889 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c We haven't been axing stuff out of intel_display.c so we could add somethign else back! ;) A new file for watermarks or display pm? Ville? BR, Jani. > @@ -125,6 +125,46 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state); > static void intel_modeset_setup_hw_state(struct drm_device *dev, > struct drm_modeset_acquire_ctx *ctx); > > + > +/** > + * intel_update_watermarks - update FIFO watermark values based on current modes > + * @crtc: the #intel_crtc on which to compute the WM > + * > + * Calculate watermark values for the various WM regs based on current mode > + * and plane configuration. > + * > + * There are several cases to deal with here: > + * - normal (i.e. non-self-refresh) > + * - self-refresh (SR) mode > + * - lines are large relative to FIFO size (buffer can hold up to 2) > + * - lines are small relative to FIFO size (buffer can hold more than 2 > + * lines), so need to account for TLB latency > + * > + * The normal calculation is: > + * watermark = dotclock * bytes per pixel * latency > + * where latency is platform & configuration dependent (we assume pessimal > + * values here). > + * > + * The SR calculation is: > + * watermark = (trunc(latency/line time)+1) * surface width * > + * bytes per pixel > + * where > + * line time = htotal / dotclock > + * surface width = hdisplay for normal plane and 64 for cursor > + * and latency is assumed to be high, as above. > + * > + * The final value programmed to the register should always be rounded up, > + * and include an extra 2 entries to account for clock crossings. > + * > + * We don't use the sprite, so we can ignore that. And on Crestline we have > + * to set the non-SR watermarks to 8. > + */ > +static void intel_update_watermarks(struct drm_i915_private *dev_priv) > +{ > + if (dev_priv->display.update_wm) > + dev_priv->display.update_wm(dev_priv); > +} > + > /* returns HPLL frequency in kHz */ > int vlv_get_hpll_vco(struct drm_i915_private *dev_priv) > { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 406baa49e6ad..4054c6f7a2f9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -7132,45 +7132,6 @@ void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv) > !(intel_uncore_read(&dev_priv->uncore, DISP_ARB_CTL) & DISP_FBC_WM_DIS); > } > > -/** > - * intel_update_watermarks - update FIFO watermark values based on current modes > - * @crtc: the #intel_crtc on which to compute the WM > - * > - * Calculate watermark values for the various WM regs based on current mode > - * and plane configuration. > - * > - * There are several cases to deal with here: > - * - normal (i.e. non-self-refresh) > - * - self-refresh (SR) mode > - * - lines are large relative to FIFO size (buffer can hold up to 2) > - * - lines are small relative to FIFO size (buffer can hold more than 2 > - * lines), so need to account for TLB latency > - * > - * The normal calculation is: > - * watermark = dotclock * bytes per pixel * latency > - * where latency is platform & configuration dependent (we assume pessimal > - * values here). > - * > - * The SR calculation is: > - * watermark = (trunc(latency/line time)+1) * surface width * > - * bytes per pixel > - * where > - * line time = htotal / dotclock > - * surface width = hdisplay for normal plane and 64 for cursor > - * and latency is assumed to be high, as above. > - * > - * The final value programmed to the register should always be rounded up, > - * and include an extra 2 entries to account for clock crossings. > - * > - * We don't use the sprite, so we can ignore that. And on Crestline we have > - * to set the non-SR watermarks to 8. > - */ > -void intel_update_watermarks(struct drm_i915_private *dev_priv) > -{ > - if (dev_priv->display.update_wm) > - dev_priv->display.update_wm(dev_priv); > -} > - > void intel_enable_ipc(struct drm_i915_private *dev_priv) > { > u32 val; > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > index 99bce0b4f5fb..990cdcaf85ce 100644 > --- a/drivers/gpu/drm/i915/intel_pm.h > +++ b/drivers/gpu/drm/i915/intel_pm.h > @@ -29,7 +29,6 @@ struct skl_wm_level; > void intel_init_clock_gating(struct drm_i915_private *dev_priv); > void intel_suspend_hw(struct drm_i915_private *dev_priv); > int ilk_wm_max_level(const struct drm_i915_private *dev_priv); > -void intel_update_watermarks(struct drm_i915_private *dev_priv); > void intel_init_pm(struct drm_i915_private *dev_priv); > void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv); > void intel_pm_setup(struct drm_i915_private *dev_priv);
On Wed, 8 Sept 2021 at 19:33, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Wed, 08 Sep 2021, Dave Airlie <airlied@gmail.com> wrote: > > From: Dave Airlie <airlied@redhat.com> > > > > A vague goal is to have the vfunc table be the api between > > wm and display, not having direction function calls cross > > the boundary. > > > > This aligns the legacy update_wm with the newer vfuncs. > > > > The comment probably needs to live somewhere else, it seems > > like it should live in the pm side though not the display side, > > but I brought it along for the ride. > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 40 ++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 39 ------------------- > > drivers/gpu/drm/i915/intel_pm.h | 1 - > > 3 files changed, 40 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index d95283bf2631..b495371c1889 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > We haven't been axing stuff out of intel_display.c so we could add > somethign else back! ;) > > A new file for watermarks or display pm? Ville? The main reason I landed it there, was because all the other calls to the wm funcs are in intel_display, and this wrapper is very small and ends up being a static, the comment on the other hand, I've no idea where it should have landed. Dave.
On Thu, Sep 09, 2021 at 06:40:59AM +1000, Dave Airlie wrote: > On Wed, 8 Sept 2021 at 19:33, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > On Wed, 08 Sep 2021, Dave Airlie <airlied@gmail.com> wrote: > > > From: Dave Airlie <airlied@redhat.com> > > > > > > A vague goal is to have the vfunc table be the api between > > > wm and display, not having direction function calls cross > > > the boundary. > > > > > > This aligns the legacy update_wm with the newer vfuncs. > > > > > > The comment probably needs to live somewhere else, it seems > > > like it should live in the pm side though not the display side, > > > but I brought it along for the ride. > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 40 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_pm.c | 39 ------------------- > > > drivers/gpu/drm/i915/intel_pm.h | 1 - > > > 3 files changed, 40 insertions(+), 40 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index d95283bf2631..b495371c1889 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > We haven't been axing stuff out of intel_display.c so we could add > > somethign else back! ;) > > > > A new file for watermarks or display pm? Ville? We need multiple files. But I've been hoping to land more watermark refactoring first so I'd not have to rebase tons of stuff across massive code motion patches. Unfortunatley review for that stuff is hard to come by. Regarding the .update_wm() hook in particular, it's just an ancient thing that is not meant to exist once all the wm code gets atomized. So no real point in polishing it any further in its current form IMO. > > The main reason I landed it there, was because all the other calls to > the wm funcs are in intel_display, and this wrapper is very small and > ends up being a static, the comment on the other hand, I've no idea > where it should have landed. > > Dave.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index d95283bf2631..b495371c1889 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -125,6 +125,46 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state); static void intel_modeset_setup_hw_state(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); + +/** + * intel_update_watermarks - update FIFO watermark values based on current modes + * @crtc: the #intel_crtc on which to compute the WM + * + * Calculate watermark values for the various WM regs based on current mode + * and plane configuration. + * + * There are several cases to deal with here: + * - normal (i.e. non-self-refresh) + * - self-refresh (SR) mode + * - lines are large relative to FIFO size (buffer can hold up to 2) + * - lines are small relative to FIFO size (buffer can hold more than 2 + * lines), so need to account for TLB latency + * + * The normal calculation is: + * watermark = dotclock * bytes per pixel * latency + * where latency is platform & configuration dependent (we assume pessimal + * values here). + * + * The SR calculation is: + * watermark = (trunc(latency/line time)+1) * surface width * + * bytes per pixel + * where + * line time = htotal / dotclock + * surface width = hdisplay for normal plane and 64 for cursor + * and latency is assumed to be high, as above. + * + * The final value programmed to the register should always be rounded up, + * and include an extra 2 entries to account for clock crossings. + * + * We don't use the sprite, so we can ignore that. And on Crestline we have + * to set the non-SR watermarks to 8. + */ +static void intel_update_watermarks(struct drm_i915_private *dev_priv) +{ + if (dev_priv->display.update_wm) + dev_priv->display.update_wm(dev_priv); +} + /* returns HPLL frequency in kHz */ int vlv_get_hpll_vco(struct drm_i915_private *dev_priv) { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 406baa49e6ad..4054c6f7a2f9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -7132,45 +7132,6 @@ void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv) !(intel_uncore_read(&dev_priv->uncore, DISP_ARB_CTL) & DISP_FBC_WM_DIS); } -/** - * intel_update_watermarks - update FIFO watermark values based on current modes - * @crtc: the #intel_crtc on which to compute the WM - * - * Calculate watermark values for the various WM regs based on current mode - * and plane configuration. - * - * There are several cases to deal with here: - * - normal (i.e. non-self-refresh) - * - self-refresh (SR) mode - * - lines are large relative to FIFO size (buffer can hold up to 2) - * - lines are small relative to FIFO size (buffer can hold more than 2 - * lines), so need to account for TLB latency - * - * The normal calculation is: - * watermark = dotclock * bytes per pixel * latency - * where latency is platform & configuration dependent (we assume pessimal - * values here). - * - * The SR calculation is: - * watermark = (trunc(latency/line time)+1) * surface width * - * bytes per pixel - * where - * line time = htotal / dotclock - * surface width = hdisplay for normal plane and 64 for cursor - * and latency is assumed to be high, as above. - * - * The final value programmed to the register should always be rounded up, - * and include an extra 2 entries to account for clock crossings. - * - * We don't use the sprite, so we can ignore that. And on Crestline we have - * to set the non-SR watermarks to 8. - */ -void intel_update_watermarks(struct drm_i915_private *dev_priv) -{ - if (dev_priv->display.update_wm) - dev_priv->display.update_wm(dev_priv); -} - void intel_enable_ipc(struct drm_i915_private *dev_priv) { u32 val; diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h index 99bce0b4f5fb..990cdcaf85ce 100644 --- a/drivers/gpu/drm/i915/intel_pm.h +++ b/drivers/gpu/drm/i915/intel_pm.h @@ -29,7 +29,6 @@ struct skl_wm_level; void intel_init_clock_gating(struct drm_i915_private *dev_priv); void intel_suspend_hw(struct drm_i915_private *dev_priv); int ilk_wm_max_level(const struct drm_i915_private *dev_priv); -void intel_update_watermarks(struct drm_i915_private *dev_priv); void intel_init_pm(struct drm_i915_private *dev_priv); void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv); void intel_pm_setup(struct drm_i915_private *dev_priv);
From: Dave Airlie <airlied@redhat.com> A vague goal is to have the vfunc table be the api between wm and display, not having direction function calls cross the boundary. This aligns the legacy update_wm with the newer vfuncs. The comment probably needs to live somewhere else, it seems like it should live in the pm side though not the display side, but I brought it along for the ride. --- drivers/gpu/drm/i915/display/intel_display.c | 40 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_pm.c | 39 ------------------- drivers/gpu/drm/i915/intel_pm.h | 1 - 3 files changed, 40 insertions(+), 40 deletions(-)