Thanks Jani!
Yes.. I was thinking the same and but got a bit mixed-up and also was in a bit of confusion how deep
these intel_display changes should go!
I see that you have added comment in the next patch that all of those i915 could be changed in one
function! I can update the series based on that.
Thanks
Vinod
On Tue, 2024-11-05 at 10:52 +0200, Jani Nikula wrote:
> On Tue, 05 Nov 2024, Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx> wrote:
> > Update intel_enabled_dbuf_slices_mask to use intel_display instead
> > of drm_i915_private object. This is a prepratory patch for the next
> > patch in the series, where all intel_de_read calls in skl_watermarks.c
> > are updated to use intel_display instead of drm_i915_private.
> >
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 +-
> > drivers/gpu/drm/i915/display/skl_watermark.c | 9 +++++----
> > drivers/gpu/drm/i915/display/skl_watermark.h | 3 ++-
> > 4 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 2766fd9208b0..62e0faffca40 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -1090,7 +1090,7 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
> > u8 slices_mask;
> >
> > dev_priv->display.dbuf.enabled_slices =
> > - intel_enabled_dbuf_slices_mask(dev_priv);
> > + intel_enabled_dbuf_slices_mask(&dev_priv->display);
>
> Please add a local struct intel_display *display variable and pass that
> to intel_enabled_dbuf_slices_mask().
>
> The point is, all of the i915/dev_priv references need to go, and if you
> add &dev_priv->display, this line needs to be updated again.
>
> >
> > slices_mask = BIT(DBUF_S1) | dev_priv->display.dbuf.enabled_slices;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > index f0131dd853de..f792db191fcf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > @@ -973,7 +973,7 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private
> > *dev_priv,
> >
> > static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> > {
> > - u8 hw_enabled_dbuf_slices = intel_enabled_dbuf_slices_mask(dev_priv);
> > + u8 hw_enabled_dbuf_slices = intel_enabled_dbuf_slices_mask(&dev_priv->display);
>
> Ditto.
>
> > u8 enabled_dbuf_slices = dev_priv->display.dbuf.enabled_slices;
> >
> > drm_WARN(&dev_priv->drm,
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 3b0e87edbacf..d9d7238f0fb4 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -52,13 +52,13 @@ struct skl_wm_params {
> > u32 dbuf_block_size;
> > };
> >
> > -u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *i915)
> > +u8 intel_enabled_dbuf_slices_mask(struct intel_display *display)
> > {
> > u8 enabled_slices = 0;
> > enum dbuf_slice slice;
> >
> > - for_each_dbuf_slice(i915, slice) {
> > - if (intel_de_read(i915, DBUF_CTL_S(slice)) & DBUF_POWER_STATE)
> > + for_each_dbuf_slice(display, slice) {
> > + if (intel_de_read(display, DBUF_CTL_S(slice)) & DBUF_POWER_STATE)
> > enabled_slices |= BIT(slice);
> > }
> >
> > @@ -3126,6 +3126,7 @@ void intel_wm_state_verify(struct intel_atomic_state *state,
> > struct intel_crtc *crtc)
> > {
> > struct drm_i915_private *i915 = to_i915(state->base.dev);
> > + struct intel_display *display = to_intel_display(state);
>
> Please prefer putting the display variable first if at all possible.
>
>
> > const struct intel_crtc_state *new_crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > struct skl_hw_state {
> > @@ -3149,7 +3150,7 @@ void intel_wm_state_verify(struct intel_atomic_state *state,
> >
> > skl_pipe_ddb_get_hw_state(crtc, hw->ddb, hw->ddb_y);
> >
> > - hw_enabled_slices = intel_enabled_dbuf_slices_mask(i915);
> > + hw_enabled_slices = intel_enabled_dbuf_slices_mask(display);
> >
> > if (DISPLAY_VER(i915) >= 11 &&
> > hw_enabled_slices != i915->display.dbuf.enabled_slices)
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h
> > b/drivers/gpu/drm/i915/display/skl_watermark.h
> > index e73baec94873..990793e36272 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> > @@ -17,11 +17,12 @@ struct intel_atomic_state;
> > struct intel_bw_state;
> > struct intel_crtc;
> > struct intel_crtc_state;
> > +struct intel_display;
> > struct intel_plane;
> > struct skl_pipe_wm;
> > struct skl_wm_level;
> >
> > -u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *i915);
> > +u8 intel_enabled_dbuf_slices_mask(struct intel_display *display);
> >
> > void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > void intel_sagv_post_plane_update(struct intel_atomic_state *state);
>
