drm/i915: align dumb buffer stride to page_sz of the region
diff mbox series

Message ID 20200205114019.10900-1-ramalingam.c@intel.com
State New
Headers show
Series
  • drm/i915: align dumb buffer stride to page_sz of the region
Related show

Commit Message

Ramalingam C Feb. 5, 2020, 11:40 a.m. UTC
If stride of the dumb buffer requested is greater than the primary
plane's max stride, then we align the stride to the page size. But the
page size was hard coded for 4096.

With the lmem addition, lets align the stride to the page size of the
memory region that will be used for dumb buffer.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Chris Wilson Feb. 5, 2020, 11:43 a.m. UTC | #1
Quoting Ramalingam C (2020-02-05 11:40:19)
> If stride of the dumb buffer requested is greater than the primary
> plane's max stride, then we align the stride to the page size. But the
> page size was hard coded for 4096.
> 
> With the lmem addition, lets align the stride to the page size of the
> memory region that will be used for dumb buffer.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a712e60b016a..0f01396ca24e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -239,8 +239,9 @@ i915_gem_dumb_create(struct drm_file *file,
>                      struct drm_device *dev,
>                      struct drm_mode_create_dumb *args)
>  {
> -       enum intel_memory_type mem_type;
>         int cpp = DIV_ROUND_UP(args->bpp, 8);
> +       enum intel_memory_type mem_type;
> +       struct intel_memory_region *mr;
>         u32 format;
>  
>         switch (cpp) {
> @@ -260,24 +261,21 @@ i915_gem_dumb_create(struct drm_file *file,
>         /* have to work out size/pitch and return them */
>         args->pitch = ALIGN(args->width * cpp, 64);
>  
> +       mem_type = INTEL_MEMORY_SYSTEM;
> +       if (HAS_LMEM(to_i915(dev)))
> +               mem_type = INTEL_MEMORY_LOCAL;
> +       mr = intel_memory_region_by_type(to_i915(dev), mem_type);
> +
>         /* align stride to page size so that we can remap */
>         if (args->pitch > intel_plane_fb_max_stride(to_i915(dev), format,
>                                                     DRM_FORMAT_MOD_LINEAR))
> -               args->pitch = ALIGN(args->pitch, 4096);
> +               args->pitch = ALIGN(args->pitch, mr->min_page_size);

That should be ggtt-page size, different semantics, right?
-Chris
Ramalingam C Feb. 5, 2020, 12:02 p.m. UTC | #2
On 2020-02-05 at 11:43:35 +0000, Chris Wilson wrote:
> Quoting Ramalingam C (2020-02-05 11:40:19)
> > If stride of the dumb buffer requested is greater than the primary
> > plane's max stride, then we align the stride to the page size. But the
> > page size was hard coded for 4096.
> > 
> > With the lmem addition, lets align the stride to the page size of the
> > memory region that will be used for dumb buffer.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index a712e60b016a..0f01396ca24e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -239,8 +239,9 @@ i915_gem_dumb_create(struct drm_file *file,
> >                      struct drm_device *dev,
> >                      struct drm_mode_create_dumb *args)
> >  {
> > -       enum intel_memory_type mem_type;
> >         int cpp = DIV_ROUND_UP(args->bpp, 8);
> > +       enum intel_memory_type mem_type;
> > +       struct intel_memory_region *mr;
> >         u32 format;
> >  
> >         switch (cpp) {
> > @@ -260,24 +261,21 @@ i915_gem_dumb_create(struct drm_file *file,
> >         /* have to work out size/pitch and return them */
> >         args->pitch = ALIGN(args->width * cpp, 64);
> >  
> > +       mem_type = INTEL_MEMORY_SYSTEM;
> > +       if (HAS_LMEM(to_i915(dev)))
> > +               mem_type = INTEL_MEMORY_LOCAL;
> > +       mr = intel_memory_region_by_type(to_i915(dev), mem_type);
> > +
> >         /* align stride to page size so that we can remap */
> >         if (args->pitch > intel_plane_fb_max_stride(to_i915(dev), format,
> >                                                     DRM_FORMAT_MOD_LINEAR))
> > -               args->pitch = ALIGN(args->pitch, 4096);
> > +               args->pitch = ALIGN(args->pitch, mr->min_page_size);
> 
> That should be ggtt-page size, different semantics, right?
Chris,

Sicne the purpose(remapping stride) is not clear, couldn't understand
which page size this is. I assumed this is of hw page size.

Btw, there is no issues found with 4096 on lmem too. May be this is
unwanted change, unless they meant hw page size here and luckily we are
not hitting the issue. I am not sure though.

-Ram
> -Chris
Chris Wilson Feb. 5, 2020, 12:05 p.m. UTC | #3
Quoting Ramalingam C (2020-02-05 12:02:50)
> On 2020-02-05 at 11:43:35 +0000, Chris Wilson wrote:
> > Quoting Ramalingam C (2020-02-05 11:40:19)
> > > If stride of the dumb buffer requested is greater than the primary
> > > plane's max stride, then we align the stride to the page size. But the
> > > page size was hard coded for 4096.
> > > 
> > > With the lmem addition, lets align the stride to the page size of the
> > > memory region that will be used for dumb buffer.
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++-----------
> > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index a712e60b016a..0f01396ca24e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -239,8 +239,9 @@ i915_gem_dumb_create(struct drm_file *file,
> > >                      struct drm_device *dev,
> > >                      struct drm_mode_create_dumb *args)
> > >  {
> > > -       enum intel_memory_type mem_type;
> > >         int cpp = DIV_ROUND_UP(args->bpp, 8);
> > > +       enum intel_memory_type mem_type;
> > > +       struct intel_memory_region *mr;
> > >         u32 format;
> > >  
> > >         switch (cpp) {
> > > @@ -260,24 +261,21 @@ i915_gem_dumb_create(struct drm_file *file,
> > >         /* have to work out size/pitch and return them */
> > >         args->pitch = ALIGN(args->width * cpp, 64);
> > >  
> > > +       mem_type = INTEL_MEMORY_SYSTEM;
> > > +       if (HAS_LMEM(to_i915(dev)))
> > > +               mem_type = INTEL_MEMORY_LOCAL;
> > > +       mr = intel_memory_region_by_type(to_i915(dev), mem_type);
> > > +
> > >         /* align stride to page size so that we can remap */
> > >         if (args->pitch > intel_plane_fb_max_stride(to_i915(dev), format,
> > >                                                     DRM_FORMAT_MOD_LINEAR))
> > > -               args->pitch = ALIGN(args->pitch, 4096);
> > > +               args->pitch = ALIGN(args->pitch, mr->min_page_size);
> > 
> > That should be ggtt-page size, different semantics, right?
> Chris,
> 
> Sicne the purpose(remapping stride) is not clear, couldn't understand
> which page size this is. I assumed this is of hw page size.
> 
> Btw, there is no issues found with 4096 on lmem too. May be this is
> unwanted change, unless they meant hw page size here and luckily we are
> not hitting the issue. I am not sure though.

It for doing tricks with I915_GGTT_VIEW_REMAPPED (intel_remap_pages) so
that a large framebuffer can be cut into CRTC-sized chunks and not go
over the HW limits.
-Chris
Ramalingam C Feb. 5, 2020, 1:47 p.m. UTC | #4
On 2020-02-05 at 12:05:11 +0000, Chris Wilson wrote:
> Quoting Ramalingam C (2020-02-05 12:02:50)
> > On 2020-02-05 at 11:43:35 +0000, Chris Wilson wrote:
> > > Quoting Ramalingam C (2020-02-05 11:40:19)
> > > > If stride of the dumb buffer requested is greater than the primary
> > > > plane's max stride, then we align the stride to the page size. But the
> > > > page size was hard coded for 4096.
> > > > 
> > > > With the lmem addition, lets align the stride to the page size of the
> > > > memory region that will be used for dumb buffer.
> > > > 
> > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++-----------
> > > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index a712e60b016a..0f01396ca24e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -239,8 +239,9 @@ i915_gem_dumb_create(struct drm_file *file,
> > > >                      struct drm_device *dev,
> > > >                      struct drm_mode_create_dumb *args)
> > > >  {
> > > > -       enum intel_memory_type mem_type;
> > > >         int cpp = DIV_ROUND_UP(args->bpp, 8);
> > > > +       enum intel_memory_type mem_type;
> > > > +       struct intel_memory_region *mr;
> > > >         u32 format;
> > > >  
> > > >         switch (cpp) {
> > > > @@ -260,24 +261,21 @@ i915_gem_dumb_create(struct drm_file *file,
> > > >         /* have to work out size/pitch and return them */
> > > >         args->pitch = ALIGN(args->width * cpp, 64);
> > > >  
> > > > +       mem_type = INTEL_MEMORY_SYSTEM;
> > > > +       if (HAS_LMEM(to_i915(dev)))
> > > > +               mem_type = INTEL_MEMORY_LOCAL;
> > > > +       mr = intel_memory_region_by_type(to_i915(dev), mem_type);
> > > > +
> > > >         /* align stride to page size so that we can remap */
> > > >         if (args->pitch > intel_plane_fb_max_stride(to_i915(dev), format,
> > > >                                                     DRM_FORMAT_MOD_LINEAR))
> > > > -               args->pitch = ALIGN(args->pitch, 4096);
> > > > +               args->pitch = ALIGN(args->pitch, mr->min_page_size);
> > > 
> > > That should be ggtt-page size, different semantics, right?
> > Chris,
> > 
> > Sicne the purpose(remapping stride) is not clear, couldn't understand
> > which page size this is. I assumed this is of hw page size.
> > 
> > Btw, there is no issues found with 4096 on lmem too. May be this is
> > unwanted change, unless they meant hw page size here and luckily we are
> > not hitting the issue. I am not sure though.
> 
> It for doing tricks with I915_GGTT_VIEW_REMAPPED (intel_remap_pages) so
> that a large framebuffer can be cut into CRTC-sized chunks and not go
> over the HW limits.

Sure. I will discard this change. Thanks for explaining that to me.

-Ram.
> -Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a712e60b016a..0f01396ca24e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -239,8 +239,9 @@  i915_gem_dumb_create(struct drm_file *file,
 		     struct drm_device *dev,
 		     struct drm_mode_create_dumb *args)
 {
-	enum intel_memory_type mem_type;
 	int cpp = DIV_ROUND_UP(args->bpp, 8);
+	enum intel_memory_type mem_type;
+	struct intel_memory_region *mr;
 	u32 format;
 
 	switch (cpp) {
@@ -260,24 +261,21 @@  i915_gem_dumb_create(struct drm_file *file,
 	/* have to work out size/pitch and return them */
 	args->pitch = ALIGN(args->width * cpp, 64);
 
+	mem_type = INTEL_MEMORY_SYSTEM;
+	if (HAS_LMEM(to_i915(dev)))
+		mem_type = INTEL_MEMORY_LOCAL;
+	mr = intel_memory_region_by_type(to_i915(dev), mem_type);
+
 	/* align stride to page size so that we can remap */
 	if (args->pitch > intel_plane_fb_max_stride(to_i915(dev), format,
 						    DRM_FORMAT_MOD_LINEAR))
-		args->pitch = ALIGN(args->pitch, 4096);
+		args->pitch = ALIGN(args->pitch, mr->min_page_size);
 
 	if (args->pitch < args->width)
 		return -EINVAL;
 
 	args->size = mul_u32_u32(args->pitch, args->height);
-
-	mem_type = INTEL_MEMORY_SYSTEM;
-	if (HAS_LMEM(to_i915(dev)))
-		mem_type = INTEL_MEMORY_LOCAL;
-
-	return i915_gem_create(file,
-			       intel_memory_region_by_type(to_i915(dev),
-							   mem_type),
-			       &args->size, &args->handle);
+	return i915_gem_create(file, mr, &args->size, &args->handle);
 }
 
 /**