Re: [PATCH 1/5] drm/i915: Move VT-d alignment into plane->min_alignment()

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

On Mon, Jan 27, 2025 at 11:50:34AM +0200, Jani Nikula wrote:
> On Wed, 22 Jan 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently we don't account for the VT-d alignment w/a in
> > plane->min_alignment() which means that panning inside a larger
> > framebuffer can still cause the plane SURF to be misaligned.
> > Fix the issue by moving the VT-d alignment w/a into
> > plane->min_alignment() itself (for the affected platforms).
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/i9xx_plane.c          | 10 ++++++++++
> >  drivers/gpu/drm/i915/display/intel_cursor.c        |  5 +++++
> >  drivers/gpu/drm/i915/display/intel_fb_pin.c        |  8 --------
> >  drivers/gpu/drm/i915/display/intel_sprite.c        |  5 +++++
> >  drivers/gpu/drm/i915/display/skl_universal_plane.c |  4 ++++
> >  5 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index ed171fbf8720..19cc34babef3 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -780,9 +780,14 @@ unsigned int vlv_plane_min_alignment(struct intel_plane *plane,
> >  				     const struct drm_framebuffer *fb,
> >  				     int color_plane)
> >  {
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> >  	if (intel_plane_can_async_flip(plane, fb->modifier))
> >  		return 256 * 1024;
> >  
> > +	if (intel_scanout_needs_vtd_wa(i915))
> > +		return 256 * 1024;
> 
> Nitpick, would be great to convert intel_scanout_needs_vtd_wa() to
> struct intel_display first, so we wouldn't have to introduce so many new
> struct drm_i915_private uses.

I didn't really want to add intel_display stuff to the
gem side (where this is being used currently). Once
its all moved into the display code (patch 3) then it
makes more sense.

> 
> BR,
> Jani.
> 
> > +
> >  	switch (fb->modifier) {
> >  	case I915_FORMAT_MOD_X_TILED:
> >  		return 4 * 1024;
> > @@ -798,9 +803,14 @@ static unsigned int g4x_primary_min_alignment(struct intel_plane *plane,
> >  					      const struct drm_framebuffer *fb,
> >  					      int color_plane)
> >  {
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> >  	if (intel_plane_can_async_flip(plane, fb->modifier))
> >  		return 256 * 1024;
> >  
> > +	if (intel_scanout_needs_vtd_wa(i915))
> > +		return 256 * 1024;
> > +
> >  	switch (fb->modifier) {
> >  	case I915_FORMAT_MOD_X_TILED:
> >  	case DRM_FORMAT_MOD_LINEAR:
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index ae7243ad6e0c..9157825e370b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -372,6 +372,11 @@ static unsigned int i9xx_cursor_min_alignment(struct intel_plane *plane,
> >  					      const struct drm_framebuffer *fb,
> >  					      int color_plane)
> >  {
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> > +	if (intel_scanout_needs_vtd_wa(i915))
> > +		return 256 * 1024;
> > +
> >  	return 4 * 1024; /* physical for i915/i945 */
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > index dd3ac7f98dfc..2b9ad46eaef7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > @@ -126,14 +126,6 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> >  	if (drm_WARN_ON(dev, alignment && !is_power_of_2(alignment)))
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	/* Note that the w/a also requires 64 PTE of padding following the
> > -	 * bo. We currently fill all unused PTE with the shadow page and so
> > -	 * we should always have valid PTE following the scanout preventing
> > -	 * the VT-d warning.
> > -	 */
> > -	if (intel_scanout_needs_vtd_wa(dev_priv) && alignment < 256 * 1024)
> > -		alignment = 256 * 1024;
> > -
> >  	/*
> >  	 * Global gtt pte registers are special registers which actually forward
> >  	 * writes to a chunk of system memory. Which means that there is no risk
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 13996d7059ad..d63e71fe469e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -980,6 +980,11 @@ static unsigned int g4x_sprite_min_alignment(struct intel_plane *plane,
> >  					     const struct drm_framebuffer *fb,
> >  					     int color_plane)
> >  {
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> > +	if (intel_scanout_needs_vtd_wa(i915))
> > +		return 256 * 1024;
> > +
> >  	return 4 * 1024;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 450dd8c64e0c..5cec2df0baca 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -649,6 +649,10 @@ static u32 skl_plane_min_alignment(struct intel_plane *plane,
> >  	if (color_plane != 0)
> >  		return 4 * 1024;
> >  
> > +	/*
> > +	 * VT-d needs at least 256k alignment,
> > +	 * but that's already covered below.
> > +	 */
> >  	switch (fb->modifier) {
> >  	case DRM_FORMAT_MOD_LINEAR:
> >  	case I915_FORMAT_MOD_X_TILED:
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel