Re: [PATCH 1/8] drm/i915/display: update intel_enabled_dbuf_slices_mask to use intel_display

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

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@intel.com> 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@intel.com>
> > ---
> >  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);
>