[PATCH v1 1/2] drm/i915/display: use PAGE_SIZE macro for FBC cfb alloc

[Govindapillai, Vinod]

On Thu, 2024-01-11 at 15:59 +0200, Ville Syrjälä wrote:
> On Thu, Jan 11, 2024 at 01:47:02PM +0000, Govindapillai, Vinod wrote:
> > Hi Ville
> > 
> > The fix is in the next patch.
> > 
> > This pach changes the 4096 to page size macro as the BUG on is based on that macro explicitly.
> 
> I think the whole PAGE_SIZE handling should be in the xe code
> since it's an implementation detail of the xe code.

Sorry.. I am not sure if I get your point correctly! I just changed the magic number with PAGE_SIZE
because in the subsequent "drm_gem_private_object_init()", the BUG_ON is based on "PAGE_SIZE"
explicitly and not 4096 
BUG_ON((size & (PAGE_SIZE - 1)) != 0);

The next patch in the series https://patchwork.freedesktop.org/patch/573909/?series=128425&rev=1 .
handles alignment in xe code

static inline int i915_gem_stolen_insert_node_in_range(struct xe_device *xe,
 	int err;
 	u32 flags = XE_BO_CREATE_PINNED_BIT | XE_BO_CREATE_STOLEN_BIT;
 
+	if (align)
+		size = ALIGN(size, align);
+


As per some comments in the xe_bo handling, it expects the userspace to be aware of the alignment
restrictions and handle the alignments return error in such cased. For kernel, it doesnt explicitly
handle anything and fails at the BUG_ON() eventually of the size is no page aligned.

> 
> > 
> > Br
> > Vinod
> > 
> > ________________________________
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Sent: Thursday, January 11, 2024 3:44:22 pm
> > To: Govindapillai, Vinod <vinod.govindapillai at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org <intel-gfx at lists.freedesktop.org>; Syrjala, Ville
> > <ville.syrjala at intel.com>
> > Subject: Re: [PATCH v1 1/2] drm/i915/display: use PAGE_SIZE macro for FBC cfb alloc
> > 
> > On Wed, Jan 10, 2024 at 01:00:08PM +0200, Vinod Govindapillai wrote:
> > > FBC compressed frame buffer size need to be PAGE_SIZE aligned
> > > and the corresponding the drm_gem functions check the object
> > > size alignment using PAGE_SIZE macro. Use the PAGE_SIZE macro
> > > in the cfb alloc as well instead of the magic number.
> > > 
> > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index f17a1afb4929..9b9c8715d664 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -764,13 +764,15 @@ static int find_compression_limit(struct intel_fbc *fbc,
> > > 
> > >        /* Try to over-allocate to reduce reallocations and fragmentation. */
> > >        ret = i915_gem_stolen_insert_node_in_range(i915, &fbc->compressed_fb,
> > > -                                                size <<= 1, 4096, 0, end);
> > > +                                                size <<= 1, PAGE_SIZE, 0,
> > > +                                                end);
> > >        if (ret == 0)
> > >                return limit;
> > > 
> > >        for (; limit <= intel_fbc_max_limit(i915); limit <<= 1) {
> > >                ret = i915_gem_stolen_insert_node_in_range(i915, &fbc->compressed_fb,
> > > -                                                        size >>= 1, 4096, 0, end);
> > > +                                                        size >>= 1, PAGE_SIZE, 0,
> > > +                                                        end);
> > 
> > PAGE_SIZE is 4k so I can't see this doing anything at all.
> > 
> > The correct fix is probably either:
> > - fix the xe gem code to always page align the size
> > - page align it in xe's i915_gem_stolen_insert_node_in_range()
> > 
> > >                if (ret == 0)
> > >                        return limit;
> > >        }
> > > --
> > > 2.34.1
> > 
> > --
> > Ville Syrjälä
> > Intel
> > 
>