Re: [PATCH] drm/i915/display: duplicate 128-byte Y-tiling feature check

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

On Fri, Sep 26, 2025 at 05:45:24PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 26, 2025 at 12:05:38PM +0300, Jani Nikula wrote:
> > We should try to get rid of checks that depend on struct
> > drm_i915_private (or struct xe_device) in display
> > code. HAS_128_BYTE_Y_TILING() is one of them. In the interest of
> > simplicity, just duplicate the check as HAS_128B_Y_TILING() in display.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
> >  drivers/gpu/drm/i915/display/intel_fb.c             | 3 +--
> >  drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h   | 1 -
> >  3 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 0e062753cf9b..3e8d4fc862ff 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -142,6 +142,7 @@ struct intel_display_platforms {
> >  	func(overlay_needs_physical); \
> >  	func(supports_tv);
> >  
> > +#define HAS_128B_Y_TILING(__display)	(DISPLAY_VER(__display) != 2 && !(__display)->platform.i915g && !(__display)->platform.i915gm)
> 
> This whole thing will be lot simpler if we invert this into
> HAS_512B_Y_TILING(), which I actually already did but no one
> felt like reviewing it:
> https://patchwork.freedesktop.org/patch/343580/?series=70396&rev=1

Or I suppose we could keep it like this and just get rid of the gen2
check. Gen2 does have 128B Y-tile (it even has 128B X-tile). It's just
different than the 128B Y-tile on i945+, which I think is why the 
current macro is so convoluted.

I suppose I should go through the gem code there and swap everything
around to deal with the 512B Y-tile first, and then deal with the two
128B Y-tile variants (or just treat them the same if there is no
actual need to differentiate between them).

> 
> >  #define HAS_4TILE(__display)		((__display)->platform.dg2 || DISPLAY_VER(__display) >= 14)
> >  #define HAS_ASYNC_FLIPS(__display)	(DISPLAY_VER(__display) >= 5)
> >  #define HAS_AS_SDP(__display)		(DISPLAY_VER(__display) >= 13)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 69237dabdae8..f2ccc9b1175d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -777,7 +777,6 @@ unsigned int
> >  intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
> >  {
> >  	struct intel_display *display = to_intel_display(fb->dev);
> > -	struct drm_i915_private *i915 = to_i915(display->drm);
> >  	unsigned int cpp = fb->format->cpp[color_plane];
> >  
> >  	switch (fb->modifier) {
> > @@ -814,7 +813,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
> >  			return 64;
> >  		fallthrough;
> >  	case I915_FORMAT_MOD_Y_TILED:
> > -		if (DISPLAY_VER(display) == 2 || HAS_128_BYTE_Y_TILING(i915))
> > +		if (DISPLAY_VER(display) == 2 || HAS_128B_Y_TILING(display))
> >  			return 128;
> >  		else
> >  			return 512;
> > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > index b8269391bc69..be3edf20de22 100644
> > --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > @@ -36,6 +36,5 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> >  #define IS_MOBILE(xe) (xe && 0)
> >  
> >  #define HAS_FLAT_CCS(xe) (xe_device_has_flat_ccs(xe))
> > -#define HAS_128_BYTE_Y_TILING(xe) (xe || 1)
> >  
> >  #endif
> > -- 
> > 2.47.3
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel