Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

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

On Wed, May 29, 2024 at 12:37:30AM +0300, Imre Deak wrote:
> On Tue, May 28, 2024 at 10:38:06PM +0300, Ville Syrjälä wrote:
> > On Tue, May 28, 2024 at 02:27:52PM +0300, Imre Deak wrote:
> > > On Mon, May 13, 2024 at 08:59:37PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Different planes could have different alignment requirements
> > > > even for the same format/modifier. Collect the alignment
> > > > requirements across all planes capable of scanning out the
> > > > fb such that the alignment used when pinning the normal ggtt
> > > > view is satisfactory to all those planes.
> > > > 
> > > > When pinning per-plane views we only have to satisfy the
> > > > alignment requirements of the specific plane.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    |  2 ++
> > > >  drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
> > > >  4 files changed, 51 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 40d6e5f4c350..58bb65832adf 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -145,6 +145,8 @@ struct intel_framebuffer {
> > > >  	};
> > > >  
> > > >  	struct i915_address_space *dpt_vm;
> > > > +
> > > > +	unsigned int min_alignment;
> > > >  };
> > > >  
> > > >  enum intel_hotplug_state {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > index 3f3a9cd534f4..c5bae05cbbc3 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <linux/dma-resv.h>
> > > >  
> > > >  #include "i915_drv.h"
> > > > +#include "intel_atomic_plane.h"
> > > >  #include "intel_display.h"
> > > >  #include "intel_display_types.h"
> > > >  #include "intel_dpt.h"
> > > > @@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
> > > >  	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
> > > >  }
> > > >  
> > > > +static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(fb->dev);
> > > > +	struct intel_plane *plane;
> > > > +	unsigned int min_alignment = 0;
> > > > +
> > > > +	for_each_intel_plane(&i915->drm, plane) {
> > > > +		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
> > > > +			continue;
> > > > +
> > > > +		if (intel_plane_needs_physical(plane))
> > > > +			continue;
> > > > +
> > > > +		min_alignment = max(min_alignment,
> > > > +				    plane->min_alignment(plane, fb, 0));
> > > > +	}
> > > > +
> > > > +	return min_alignment;
> > > > +}
> > > > +
> > > >  int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
> > > >  {
> > > >  	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
> > > > @@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > +	fb->min_alignment = intel_fb_min_alignment(&fb->base);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > index 9b0f1ea41b70..1ae02de906f5 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > @@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> > > >  	i915_vma_put(vma);
> > > >  }
> > > >  
> > > > +static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
> > > > +{
> > > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > > > +
> > > > +	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
> > > > +	    intel_fb_needs_pot_stride_remap(fb))
> > > > +		return false;
> > > 
> > > The above view is created only once for the FB, I suppose this is how
> > > you differentiate it from views created each time due to a stride
> > > limit (based on intel_plane_needs_remap()).
> > > 
> > > > +
> > > > +	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;
> > > 
> > > So the rotated and remapped views created for a stride limit are
> > > per-plane based on the above. There is also a rotated view created only
> > > once for the FB, is it intentional to regard this kind of view as
> > > per-plane as well?
> > 
> > No. Forgot those exist. I guess I should just remove this per-plane
> > alignment logic for now. Need to think of a proper way to add it back,
> > or just quietly bury the whole idea.
> 
> Another variation is that intel_fb_pin_to_ggtt() etc. tries to bind the
> vma in max -> min alignment order considering all the plane types here
> always, though that's almost the opposite to what you want to solve
> I suppose (avoid to overalign?).
> 
> > > > +}
> > > > +
> > > >  static unsigned int
> > > >  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> > > >  {
> > > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > > >  
> > > > -	return plane->min_alignment(plane, fb, 0);
> > > > +	/*
> > > > +	 * Only use plane specific alignment for binding
> > > > +	 * a per-plane gtt view (remapped or rotated),
> > > > +	 * otherwise make sure the alignment is suitable
> > > > +	 * for all planes.
> > > > +	 */
> > > > +	if (!gtt_view_is_per_plane(plane_state))
> > > > +		return fb->min_alignment;
> > > > +
> > > > +	if (intel_plane_needs_physical(plane))
> > > > +		return 0;
> > > 
> > > I guess the above is ok, though looks like an unrelated change: the
> > > cursor plane min_alignment() for relevant platforms is <= 4k, which will
> > > be rounded up to 4k anyway when binding the vma.
> > 
> > When intel_plane_needs_physical()==true the alignment declared
> > by the plane only applies to the physical/dma address, as that
> > is what the hardware will consume. Doesn't actually matter
> > where it gets bound in the ggtt.
> 
> Ok, it's unclear why the same physical alignment was used then to bind
> the vma as well, but didn't probably matter in practice since the
> alignment was small. The commit log could still mention this change.

I guess if I just drop the per-plane tricks then we should just 
always use fb->min_alignment for the ggtt. Although I think that
is going to be end up being 0 since currently we expose the ARGB
format only for cursor planes on these platforms.

-- 
Ville Syrjälä
Intel