Re: [PATCH 2/9] drm/{i915, xe}/fbdev: add intel_fbdev_fb_pitch_align()

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

On Mon, Sep 08, 2025 at 06:10:56PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Den 2025-09-08 kl. 16:19, skrev Lucas De Marchi:
> > On Mon, Sep 08, 2025 at 03:55:18PM +0300, Jani Nikula wrote:
> >> On Fri, 05 Sep 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>> On Wed, Sep 03, 2025 at 11:31:59PM +0300, Jani Nikula wrote:
> >>>> Add new helper intel_fbdev_fb_pitch_align() in preparation for further
> >>>> refactoring. The alignment is different for i915 and xe.
> >>>>
> >>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 8 ++++++--
> >>>>  drivers/gpu/drm/i915/display/intel_fbdev_fb.h | 3 +++
> >>>>  drivers/gpu/drm/xe/display/intel_fbdev_fb.c   | 8 ++++++--
> >>>>  3 files changed, 15 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >>>> index 9c557917d781..d1c03d7b9bdc 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >>>> @@ -13,6 +13,11 @@
> >>>>  #include "intel_fb.h"
> >>>>  #include "intel_fbdev_fb.h"
> >>>>
> >>>> +u32 intel_fbdev_fb_pitch_align(u32 stride)
> >>>> +{
> >>>> +    return ALIGN(stride, 64);
> >>>> +}
> >>>> +
> >>>>  struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
> >>>>                             struct drm_fb_helper_surface_size *sizes)
> >>>>  {
> >>>> @@ -30,8 +35,7 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
> >>>>      mode_cmd.width = sizes->surface_width;
> >>>>      mode_cmd.height = sizes->surface_height;
> >>>>
> >>>> -    mode_cmd.pitches[0] = ALIGN(mode_cmd.width *
> >>>> -                    DIV_ROUND_UP(sizes->surface_bpp, 8), 64);
> >>>> +    mode_cmd.pitches[0] = intel_fbdev_fb_pitch_align(mode_cmd.width * DIV_ROUND_UP(sizes->surface_bpp, 8));
> >>>>      mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> >>>>                                sizes->surface_depth);
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >>>> index 668ae355f5e5..caeb543d5efc 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >>>> @@ -6,6 +6,8 @@
> >>>>  #ifndef __INTEL_FBDEV_FB_H__
> >>>>  #define __INTEL_FBDEV_FB_H__
> >>>>
> >>>> +#include <linux/types.h>
> >>>> +
> >>>>  struct drm_device;
> >>>>  struct drm_fb_helper_surface_size;
> >>>>  struct drm_gem_object;
> >>>> @@ -13,6 +15,7 @@ struct fb_info;
> >>>>  struct i915_vma;
> >>>>  struct intel_display;
> >>>>
> >>>> +u32 intel_fbdev_fb_pitch_align(u32 stride);
> >>>>  struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
> >>>>                             struct drm_fb_helper_surface_size *sizes);
> >>>>  int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info,
> >>>> diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >>>> index bafca1059a40..fd2c40020eea 100644
> >>>> --- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >>>> +++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >>>> @@ -15,6 +15,11 @@
> >>>>
> >>>>  #include <generated/xe_wa_oob.h>
> >>>>
> >>>> +u32 intel_fbdev_fb_pitch_align(u32 stride)
> >>>> +{
> >>>> +    return ALIGN(stride, XE_PAGE_SIZE);
> >>>
> >>> I think someone needs to explain what the heck this extra alignment
> >>> is trying to achieve? I suspect it just needs to get nuked.
> >>
> >> That would indeed be great, and allow even further cleanup. I'm just
> >> trying to find the common parts here, and not so much dig into every
> >> little detail that differs.
> >>
> >> I did git blame this while writing the patch, and it's all in the big xe
> >> driver enabling commit. I'm not sure where it comes from.
> >>
> >> Cc: Maarten, any ideas?
> > 
> > It doesn't help much, but https://lore.kernel.org/intel-xe/20231114110056.2423853-5-jouni.hogander@intel.com/
> > was the first time this align got added.
> > 
> > Adding some people to Cc.
> > 
> > Lucas De Marchi
> 
> If I really recall correctly, it was to ensure size was a multiple of 4k and dumb bo allocation would not fail.
> This was at the very start of xe enabling.
> 
> i915 bo alloc allows any size, xe is much stricter. If it works with the hack removed and the bo gets 64k aligned 
> on discrete, then this probably can be dropped.

It's aligning stride, not the size. So doesn't make sense. The only
time you need page alignment for stride is for remapping, which is
handled correctly by i915 in the dumb bo codepath and not handled at all
by xe as usual.

I suspect what we really should do for the fbdev allocation is to use
the dump bo code, and make sure that code works the same way for i915
and xe.

-- 
Ville Syrjälä
Intel