Re: [PATCH 02/20] drm/i915/fbc: Convert to intel_display, mostly

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 09, 2024 at 03:49:29PM -0400, Rodrigo Vivi wrote:
> On Fri, Jul 05, 2024 at 05:52:36PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Switch the FBC code over to intel_display from i915, as
> > much as possible. This is the future direction so that
> > the display code can be shared between i915 and xe more
> > cleanly.
> > 
> > Some of the platform checks and the stolen mem facing stiff
> > still need i915 around though.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/intel_display_debugfs.c  |   4 +-
> >  .../drm/i915/display/intel_display_driver.c   |   4 +-
> >  drivers/gpu/drm/i915/display/intel_fbc.c      | 422 ++++++++++--------
> >  drivers/gpu/drm/i915/display/intel_fbc.h      |  13 +-
> >  .../drm/i915/display/intel_fifo_underrun.c    |   2 +-
> >  .../drm/i915/display/intel_modeset_setup.c    |   2 +-
> >  6 files changed, 239 insertions(+), 208 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 91757fed9c6d..5cf9b4af9adf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -1008,7 +1008,7 @@ i915_fifo_underrun_reset_write(struct file *filp,
> >  			return ret;
> >  	}
> >  
> > -	intel_fbc_reset_underrun(dev_priv);
> > +	intel_fbc_reset_underrun(&dev_priv->display);
> >  
> >  	return cnt;
> >  }
> > @@ -1063,7 +1063,7 @@ void intel_display_debugfs_register(struct drm_i915_private *i915)
> >  	intel_bios_debugfs_register(i915);
> >  	intel_cdclk_debugfs_register(i915);
> >  	intel_dmc_debugfs_register(i915);
> > -	intel_fbc_debugfs_register(i915);
> > +	intel_fbc_debugfs_register(&i915->display);
> >  	intel_hpd_debugfs_register(i915);
> >  	intel_opregion_debugfs_register(i915);
> >  	intel_psr_debugfs_register(i915);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > index 794b4af38055..13e206ec450f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > @@ -265,7 +265,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
> >  
> >  	intel_init_quirks(display);
> >  
> > -	intel_fbc_init(i915);
> > +	intel_fbc_init(display);
> >  
> >  	return 0;
> >  
> > @@ -607,7 +607,7 @@ void intel_display_driver_remove_noirq(struct drm_i915_private *i915)
> >  	destroy_workqueue(i915->display.wq.flip);
> >  	destroy_workqueue(i915->display.wq.modeset);
> >  
> > -	intel_fbc_cleanup(i915);
> > +	intel_fbc_cleanup(&i915->display);
> >  }
> >  
> >  /* part #3: call after gem init */
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index ba9820d4b78f..de8caa69a0dd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -61,13 +61,13 @@
> >  #include "intel_fbc_regs.h"
> >  #include "intel_frontbuffer.h"
> >  
> > -#define for_each_fbc_id(__dev_priv, __fbc_id) \
> > +#define for_each_fbc_id(__display, __fbc_id) \
> >  	for ((__fbc_id) = INTEL_FBC_A; (__fbc_id) < I915_MAX_FBCS; (__fbc_id)++) \
> > -		for_each_if(DISPLAY_RUNTIME_INFO(__dev_priv)->fbc_mask & BIT(__fbc_id))
> > +		for_each_if(DISPLAY_RUNTIME_INFO(__display)->fbc_mask & BIT(__fbc_id))
> >  
> > -#define for_each_intel_fbc(__dev_priv, __fbc, __fbc_id) \
> > -	for_each_fbc_id((__dev_priv), (__fbc_id)) \
> > -		for_each_if((__fbc) = (__dev_priv)->display.fbc[(__fbc_id)])
> > +#define for_each_intel_fbc(__display, __fbc, __fbc_id) \
> > +	for_each_fbc_id((__display), (__fbc_id)) \
> > +		for_each_if((__fbc) = (__display)->fbc[(__fbc_id)])
> >  
> >  struct intel_fbc_funcs {
> >  	void (*activate)(struct intel_fbc *fbc);
> > @@ -90,7 +90,7 @@ struct intel_fbc_state {
> >  };
> >  
> >  struct intel_fbc {
> > -	struct drm_i915_private *i915;
> > +	struct intel_display *display;
> >  	const struct intel_fbc_funcs *funcs;
> >  
> >  	/*
> > @@ -151,7 +151,7 @@ static unsigned int _intel_fbc_cfb_stride(const struct intel_plane_state *plane_
> >  /* minimum acceptable cfb stride in bytes, assuming 1:1 compression limit */
> >  static unsigned int skl_fbc_min_cfb_stride(const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > +	struct intel_display *display = to_intel_display(plane_state->uapi.plane->dev);
> >  	unsigned int limit = 4; /* 1:4 compression limit is the worst case */
> >  	unsigned int cpp = 4; /* FBC always 4 bytes per pixel */
> >  	unsigned int width = drm_rect_width(&plane_state->uapi.src) >> 16;
> > @@ -165,7 +165,7 @@ static unsigned int skl_fbc_min_cfb_stride(const struct intel_plane_state *plane
> >  	 * Wa_16011863758: icl+
> >  	 * Avoid some hardware segment address miscalculation.
> >  	 */
> > -	if (DISPLAY_VER(i915) >= 11)
> > +	if (DISPLAY_VER(display) >= 11)
> >  		stride += 64;
> >  
> >  	/*
> > @@ -181,7 +181,7 @@ static unsigned int skl_fbc_min_cfb_stride(const struct intel_plane_state *plane
> >  /* properly aligned cfb stride in bytes, assuming 1:1 compression limit */
> >  static unsigned int intel_fbc_cfb_stride(const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > +	struct intel_display *display = to_intel_display(plane_state->uapi.plane->dev);
> >  	unsigned int stride = _intel_fbc_cfb_stride(plane_state);
> >  
> >  	/*
> > @@ -189,7 +189,7 @@ static unsigned int intel_fbc_cfb_stride(const struct intel_plane_state *plane_s
> >  	 * be 512 byte aligned. Aligning each line to 512 bytes guarantees
> >  	 * that regardless of the compression limit we choose later.
> >  	 */
> > -	if (DISPLAY_VER(i915) >= 9)
> > +	if (DISPLAY_VER(display) >= 9)
> >  		return max(ALIGN(stride, 512), skl_fbc_min_cfb_stride(plane_state));
> >  	else
> >  		return stride;
> > @@ -197,12 +197,12 @@ static unsigned int intel_fbc_cfb_stride(const struct intel_plane_state *plane_s
> >  
> >  static unsigned int intel_fbc_cfb_size(const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > +	struct intel_display *display = to_intel_display(plane_state->uapi.plane->dev);
> >  	int lines = drm_rect_height(&plane_state->uapi.src) >> 16;
> >  
> > -	if (DISPLAY_VER(i915) == 7)
> > +	if (DISPLAY_VER(display) == 7)
> >  		lines = min(lines, 2048);
> > -	else if (DISPLAY_VER(i915) >= 8)
> > +	else if (DISPLAY_VER(display) >= 8)
> >  		lines = min(lines, 2560);
> >  
> >  	return lines * intel_fbc_cfb_stride(plane_state);
> > @@ -210,7 +210,7 @@ static unsigned int intel_fbc_cfb_size(const struct intel_plane_state *plane_sta
> >  
> >  static u16 intel_fbc_override_cfb_stride(const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > +	struct intel_display *display = to_intel_display(plane_state->uapi.plane->dev);
> >  	unsigned int stride_aligned = intel_fbc_cfb_stride(plane_state);
> >  	unsigned int stride = _intel_fbc_cfb_stride(plane_state);
> >  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > @@ -223,28 +223,31 @@ static u16 intel_fbc_override_cfb_stride(const struct intel_plane_state *plane_s
> >  	 * we always need to use the override there.
> >  	 */
> >  	if (stride != stride_aligned ||
> > -	    (DISPLAY_VER(i915) == 9 && fb->modifier == DRM_FORMAT_MOD_LINEAR))
> > +	    (DISPLAY_VER(display) == 9 && fb->modifier == DRM_FORMAT_MOD_LINEAR))
> >  		return stride_aligned * 4 / 64;
> >  
> >  	return 0;
> >  }
> >  
> > -static bool intel_fbc_has_fences(struct drm_i915_private *i915)
> > +static bool intel_fbc_has_fences(struct intel_display *display)
> >  {
> > +	struct drm_i915_private __maybe_unused *i915 = to_i915(display->drm);
> 
> I was going to ask why __maybe_unused,
> but then I remember about the xe compat-headers where below function is
> defined to 0...

Yaeh this one is needed.


 >  
> > -static u64 intel_fbc_stolen_end(struct drm_i915_private *i915)
> > +static u64 intel_fbc_stolen_end(struct intel_display *display)
> >  {
> > +	struct drm_i915_private __maybe_unused *i915 = to_i915(display->drm);

This one was only needed because I accidentally used to_i915() below
instead of using the local i915 variable. I adjusted this while pushing.

-- 
Ville Syrjälä
Intel