| Message ID | 20240513175942.12910-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Polish plane surface alignment handling | expand |
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? > +} > + > 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. > + > + return plane->min_alignment(plane, &fb->base, 0); The commit could've had more details about the rational for the above. As I understand it avoids having to rebind the vma for different plane types, though this is already handled to some degree by i915_vma::display_alignment. > } > > static unsigned int > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index ff685aebbd1a..124aac172acb 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -46,7 +46,6 @@ > #include "gem/i915_gem_mman.h" > > #include "i915_drv.h" > -#include "intel_crtc.h" > #include "intel_display_types.h" > #include "intel_fb.h" > #include "intel_fb_pin.h" > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = { > > __diag_pop(); > > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb) > -{ > - struct drm_i915_private *i915 = to_i915(fb->dev); > - struct intel_plane *plane; > - struct intel_crtc *crtc; > - > - crtc = intel_first_crtc(i915); > - if (!crtc) > - return 0; > - > - plane = to_intel_plane(crtc->base.primary); > - > - return plane->min_alignment(plane, fb, 0); > -} > - > static int intelfb_create(struct drm_fb_helper *helper, > struct drm_fb_helper_surface_size *sizes) > { > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > * BIOS is suitable for own access. > */ > vma = intel_fb_pin_to_ggtt(&fb->base, &view, > - intel_fbdev_min_alignment(&fb->base), 0, > + fb->min_alignment, 0, > false, &flags); > if (IS_ERR(vma)) { > ret = PTR_ERR(vma); > -- > 2.43.2 >
On Tue, May 28, 2024 at 02:27:58PM +0300, Imre Deak wrote: > [...] > > +} > > + > > 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. Hm, actually this would change the current 16k vma alignment for cursors on i830? > > > + > > + return plane->min_alignment(plane, &fb->base, 0); > > The commit could've had more details about the rational for the above. > As I understand it avoids having to rebind the vma for different plane > types, though this is already handled to some degree by > i915_vma::display_alignment. > > > } > > > > static unsigned int > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index ff685aebbd1a..124aac172acb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -46,7 +46,6 @@ > > #include "gem/i915_gem_mman.h" > > > > #include "i915_drv.h" > > -#include "intel_crtc.h" > > #include "intel_display_types.h" > > #include "intel_fb.h" > > #include "intel_fb_pin.h" > > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = { > > > > __diag_pop(); > > > > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb) > > -{ > > - struct drm_i915_private *i915 = to_i915(fb->dev); > > - struct intel_plane *plane; > > - struct intel_crtc *crtc; > > - > > - crtc = intel_first_crtc(i915); > > - if (!crtc) > > - return 0; > > - > > - plane = to_intel_plane(crtc->base.primary); > > - > > - return plane->min_alignment(plane, fb, 0); > > -} > > - > > static int intelfb_create(struct drm_fb_helper *helper, > > struct drm_fb_helper_surface_size *sizes) > > { > > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > > * BIOS is suitable for own access. > > */ > > vma = intel_fb_pin_to_ggtt(&fb->base, &view, > > - intel_fbdev_min_alignment(&fb->base), 0, > > + fb->min_alignment, 0, > > false, &flags); > > if (IS_ERR(vma)) { > > ret = PTR_ERR(vma); > > -- > > 2.43.2 > >
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. > > > +} > > + > > 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. > > > + > > + return plane->min_alignment(plane, &fb->base, 0); > > The commit could've had more details about the rational for the above. > As I understand it avoids having to rebind the vma for different plane > types, though this is already handled to some degree by > i915_vma::display_alignment. It was also semi-handled by intel_surf_alignment() trying to return the max alignment for all the planes more or less. But it was a fairly big mess. I did't really remember that the display_alignment thing even exists. Hmm. I wonder if it's even needed anymore. I suppose we can have cases where the display was previously using the vma, which was subsequency unbound, and later rebound due to some other reason. We probably want to preserve the alignment to make sure it'll again be ready to be used by the display without an extra rebind. I guess that only really happens with pre-ppgtt platforms and/or software rendering via ggtt. Hmm. We could perpaps change the implementation to adjust display_alignment when the framebuffer is created instead of doing it when the vma actually gets pinned. Might not matter for real world uses cases though. > > > } > > > > static unsigned int > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index ff685aebbd1a..124aac172acb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -46,7 +46,6 @@ > > #include "gem/i915_gem_mman.h" > > > > #include "i915_drv.h" > > -#include "intel_crtc.h" > > #include "intel_display_types.h" > > #include "intel_fb.h" > > #include "intel_fb_pin.h" > > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = { > > > > __diag_pop(); > > > > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb) > > -{ > > - struct drm_i915_private *i915 = to_i915(fb->dev); > > - struct intel_plane *plane; > > - struct intel_crtc *crtc; > > - > > - crtc = intel_first_crtc(i915); > > - if (!crtc) > > - return 0; > > - > > - plane = to_intel_plane(crtc->base.primary); > > - > > - return plane->min_alignment(plane, fb, 0); > > -} > > - > > static int intelfb_create(struct drm_fb_helper *helper, > > struct drm_fb_helper_surface_size *sizes) > > { > > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > > * BIOS is suitable for own access. > > */ > > vma = intel_fb_pin_to_ggtt(&fb->base, &view, > > - intel_fbdev_min_alignment(&fb->base), 0, > > + fb->min_alignment, 0, > > false, &flags); > > if (IS_ERR(vma)) { > > ret = PTR_ERR(vma); > > -- > > 2.43.2 > >
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. > > > + > > > + return plane->min_alignment(plane, &fb->base, 0); > > > > The commit could've had more details about the rational for the above. > > As I understand it avoids having to rebind the vma for different plane > > types, though this is already handled to some degree by > > i915_vma::display_alignment. > > It was also semi-handled by intel_surf_alignment() trying to > return the max alignment for all the planes more or less. > But it was a fairly big mess. > > I did't really remember that the display_alignment > thing even exists. Hmm. I wonder if it's even needed > anymore. I suppose we can have cases where the display was > previously using the vma, which was subsequency unbound, > and later rebound due to some other reason. We probably > want to preserve the alignment to make sure it'll again > be ready to be used by the display without an extra > rebind. I guess that only really happens with pre-ppgtt > platforms and/or software rendering via ggtt. Okay, thanks for clarifying. So the idea behind it wasn't using the same vma for multiple plane types. It makes sense to optimize the alignment upfront as done in this patch in any case and regard display_alignment as independent to this logic. > Hmm. We could perpaps change the implementation to adjust > display_alignment when the framebuffer is created instead > of doing it when the vma actually gets pinned. Might not > matter for real world uses cases though. > > > > > > } > > > > > > static unsigned int > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > index ff685aebbd1a..124aac172acb 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > @@ -46,7 +46,6 @@ > > > #include "gem/i915_gem_mman.h" > > > > > > #include "i915_drv.h" > > > -#include "intel_crtc.h" > > > #include "intel_display_types.h" > > > #include "intel_fb.h" > > > #include "intel_fb_pin.h" > > > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = { > > > > > > __diag_pop(); > > > > > > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb) > > > -{ > > > - struct drm_i915_private *i915 = to_i915(fb->dev); > > > - struct intel_plane *plane; > > > - struct intel_crtc *crtc; > > > - > > > - crtc = intel_first_crtc(i915); > > > - if (!crtc) > > > - return 0; > > > - > > > - plane = to_intel_plane(crtc->base.primary); > > > - > > > - return plane->min_alignment(plane, fb, 0); > > > -} > > > - > > > static int intelfb_create(struct drm_fb_helper *helper, > > > struct drm_fb_helper_surface_size *sizes) > > > { > > > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > > > * BIOS is suitable for own access. > > > */ > > > vma = intel_fb_pin_to_ggtt(&fb->base, &view, > > > - intel_fbdev_min_alignment(&fb->base), 0, > > > + fb->min_alignment, 0, > > > false, &flags); > > > if (IS_ERR(vma)) { > > > ret = PTR_ERR(vma); > > > -- > > > 2.43.2 > > > > > -- > Ville Syrjälä > Intel
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.
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; + + return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL; +} + 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; + + return plane->min_alignment(plane, &fb->base, 0); } static unsigned int diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index ff685aebbd1a..124aac172acb 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -46,7 +46,6 @@ #include "gem/i915_gem_mman.h" #include "i915_drv.h" -#include "intel_crtc.h" #include "intel_display_types.h" #include "intel_fb.h" #include "intel_fb_pin.h" @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = { __diag_pop(); -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb) -{ - struct drm_i915_private *i915 = to_i915(fb->dev); - struct intel_plane *plane; - struct intel_crtc *crtc; - - crtc = intel_first_crtc(i915); - if (!crtc) - return 0; - - plane = to_intel_plane(crtc->base.primary); - - return plane->min_alignment(plane, fb, 0); -} - static int intelfb_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) { @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper, * BIOS is suitable for own access. */ vma = intel_fb_pin_to_ggtt(&fb->base, &view, - intel_fbdev_min_alignment(&fb->base), 0, + fb->min_alignment, 0, false, &flags); if (IS_ERR(vma)) { ret = PTR_ERR(vma);