| Message ID | 20240110110009.28799-2-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe: ensure fbc cfb size to be page size aligned | expand |
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@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
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. Br Vinod
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. > > Br > Vinod > > ________________________________ > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, January 11, 2024 3:44:22 pm > To: Govindapillai, Vinod <vinod.govindapillai@intel.com> > Cc: intel-gfx@lists.freedesktop.org <intel-gfx@lists.freedesktop.org>; Syrjala, Ville <ville.syrjala@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@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 >
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@linux.intel.com> > > Sent: Thursday, January 11, 2024 3:44:22 pm > > To: Govindapillai, Vinod <vinod.govindapillai@intel.com> > > Cc: intel-gfx@lists.freedesktop.org <intel-gfx@lists.freedesktop.org>; Syrjala, Ville > > <ville.syrjala@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@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 > > >
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); if (ret == 0) return limit; }
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@intel.com> --- drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)