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@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > .../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
