[v4,2/2] drm/i915: Create dumb buffer from LMEM
diff mbox series

Message ID 20191202065458.9477-2-ramalingam.c@intel.com
State New
Headers show
Series
  • [v4,1/2] drm/i915: lookup for mem_region of a mem_type
Related show

Commit Message

Ramalingam C Dec. 2, 2019, 6:54 a.m. UTC
When LMEM is supported, dumb buffer preferred to be created from LMEM.

v2:
  Parameters are reshuffled. [Chris]
v3:
  s/region_id/mem_type
v4:
  use the i915_gem_object_create_region [chris]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Matthew Auld Dec. 5, 2019, 12:12 p.m. UTC | #1
On Mon, 2 Dec 2019 at 06:55, Ramalingam C <ramalingam.c@intel.com> wrote:
>
> When LMEM is supported, dumb buffer preferred to be created from LMEM.
>
> v2:
>   Parameters are reshuffled. [Chris]
> v3:
>   s/region_id/mem_type
> v4:
>   use the i915_gem_object_create_region [chris]
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> cc: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61395b03443e..34e480c8293e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -44,6 +44,7 @@
>  #include "gem/i915_gem_clflush.h"
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_ioctls.h"
> +#include "gem/i915_gem_region.h"
>  #include "gem/i915_gem_pm.h"
>  #include "gt/intel_context.h"
>  #include "gt/intel_engine_user.h"
> @@ -176,6 +177,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>  static int
>  i915_gem_create(struct drm_file *file,
>                 struct drm_i915_private *dev_priv,
> +               enum intel_memory_type mem_type,
>                 u64 *size_p,
>                 u32 *handle_p)
>  {
> @@ -189,7 +191,8 @@ i915_gem_create(struct drm_file *file,
>                 return -EINVAL;
>
>         /* Allocate the new object */
> -       obj = i915_gem_object_create_shmem(dev_priv, size);
> +       obj = i915_gem_object_create_region(intel_memory_region_lookup(dev_priv,
> +                                           mem_type), size, 0);

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

But how should we go about testing this?

We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
that uses get_avail_ram_mb() for always_clear, but maybe we need the
query region uapi for that?

>         if (IS_ERR(obj))
>                 return PTR_ERR(obj);
>
> @@ -209,6 +212,7 @@ i915_gem_dumb_create(struct drm_file *file,
>                      struct drm_device *dev,
>                      struct drm_mode_create_dumb *args)
>  {
> +       enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
>         int cpp = DIV_ROUND_UP(args->bpp, 8);
>         u32 format;
>
> @@ -235,7 +239,11 @@ i915_gem_dumb_create(struct drm_file *file,
>                 args->pitch = ALIGN(args->pitch, 4096);
>
>         args->size = args->pitch * args->height;
> -       return i915_gem_create(file, to_i915(dev),
> +
> +       if (HAS_LMEM(to_i915(dev)))
> +               mem_type = INTEL_MEMORY_LOCAL;
> +
> +       return i915_gem_create(file, to_i915(dev), mem_type,
>                                &args->size, &args->handle);
>  }
>
> @@ -254,7 +262,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>
>         i915_gem_flush_free_objects(dev_priv);
>
> -       return i915_gem_create(file, dev_priv,
> +       return i915_gem_create(file, dev_priv, INTEL_MEMORY_SYSTEM,
>                                &args->size, &args->handle);
>  }
>
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Dec. 5, 2019, 12:20 p.m. UTC | #2
Quoting Matthew Auld (2019-12-05 12:12:19)
> On Mon, 2 Dec 2019 at 06:55, Ramalingam C <ramalingam.c@intel.com> wrote:
> >
> > When LMEM is supported, dumb buffer preferred to be created from LMEM.
> >
> > v2:
> >   Parameters are reshuffled. [Chris]
> > v3:
> >   s/region_id/mem_type
> > v4:
> >   use the i915_gem_object_create_region [chris]
> >
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 61395b03443e..34e480c8293e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -44,6 +44,7 @@
> >  #include "gem/i915_gem_clflush.h"
> >  #include "gem/i915_gem_context.h"
> >  #include "gem/i915_gem_ioctls.h"
> > +#include "gem/i915_gem_region.h"
> >  #include "gem/i915_gem_pm.h"
> >  #include "gt/intel_context.h"
> >  #include "gt/intel_engine_user.h"
> > @@ -176,6 +177,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> >  static int
> >  i915_gem_create(struct drm_file *file,
> >                 struct drm_i915_private *dev_priv,
> > +               enum intel_memory_type mem_type,
> >                 u64 *size_p,
> >                 u32 *handle_p)
> >  {
> > @@ -189,7 +191,8 @@ i915_gem_create(struct drm_file *file,
> >                 return -EINVAL;
> >
> >         /* Allocate the new object */
> > -       obj = i915_gem_object_create_shmem(dev_priv, size);
> > +       obj = i915_gem_object_create_region(intel_memory_region_lookup(dev_priv,
> > +                                           mem_type), size, 0);
> 
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> 
> But how should we go about testing this?
> 
> We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> that uses get_avail_ram_mb() for always_clear, but maybe we need the
> query region uapi for that?

Hmm. Questions over the maximum size for dumb buffer, maximum number of
dumb buffers, etc, should be addressed to the dumb API. So some form of
drmGetCap() ?

Note for merging, we need the lmem vm_fault implementation first.
-Chris
Ramalingam C Dec. 5, 2019, 12:59 p.m. UTC | #3
On 2019-12-05 at 12:12:19 +0000, Matthew Auld wrote:
> On Mon, 2 Dec 2019 at 06:55, Ramalingam C <ramalingam.c@intel.com> wrote:
> >
> > When LMEM is supported, dumb buffer preferred to be created from LMEM.
> >
> > v2:
> >   Parameters are reshuffled. [Chris]
> > v3:
> >   s/region_id/mem_type
> > v4:
> >   use the i915_gem_object_create_region [chris]
> >
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 61395b03443e..34e480c8293e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -44,6 +44,7 @@
> >  #include "gem/i915_gem_clflush.h"
> >  #include "gem/i915_gem_context.h"
> >  #include "gem/i915_gem_ioctls.h"
> > +#include "gem/i915_gem_region.h"
> >  #include "gem/i915_gem_pm.h"
> >  #include "gt/intel_context.h"
> >  #include "gt/intel_engine_user.h"
> > @@ -176,6 +177,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> >  static int
> >  i915_gem_create(struct drm_file *file,
> >                 struct drm_i915_private *dev_priv,
> > +               enum intel_memory_type mem_type,
> >                 u64 *size_p,
> >                 u32 *handle_p)
> >  {
> > @@ -189,7 +191,8 @@ i915_gem_create(struct drm_file *file,
> >                 return -EINVAL;
> >
> >         /* Allocate the new object */
> > -       obj = i915_gem_object_create_shmem(dev_priv, size);
> > +       obj = i915_gem_object_create_region(intel_memory_region_lookup(dev_priv,
> > +                                           mem_type), size, 0);
> 
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> 
> But how should we go about testing this?
> 
> We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> in order to pass the IGTs.
Is this not cleared now? Tested this always clear on system memory not
yet on LMEM. Yes. we need to adjust the dumb buffer IGT to check whether
LMEM is supported? if so what is the max size that we could allocate as dumb
buffer etc.

-Ram.
> We also need to adjust dumb_buffer.c, since
> that uses get_avail_ram_mb() for always_clear, but maybe we need the
> query region uapi for that?
> 
> >         if (IS_ERR(obj))
> >                 return PTR_ERR(obj);
> >
> > @@ -209,6 +212,7 @@ i915_gem_dumb_create(struct drm_file *file,
> >                      struct drm_device *dev,
> >                      struct drm_mode_create_dumb *args)
> >  {
> > +       enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
> >         int cpp = DIV_ROUND_UP(args->bpp, 8);
> >         u32 format;
> >
> > @@ -235,7 +239,11 @@ i915_gem_dumb_create(struct drm_file *file,
> >                 args->pitch = ALIGN(args->pitch, 4096);
> >
> >         args->size = args->pitch * args->height;
> > -       return i915_gem_create(file, to_i915(dev),
> > +
> > +       if (HAS_LMEM(to_i915(dev)))
> > +               mem_type = INTEL_MEMORY_LOCAL;
> > +
> > +       return i915_gem_create(file, to_i915(dev), mem_type,
> >                                &args->size, &args->handle);
> >  }
> >
> > @@ -254,7 +262,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
> >
> >         i915_gem_flush_free_objects(dev_priv);
> >
> > -       return i915_gem_create(file, dev_priv,
> > +       return i915_gem_create(file, dev_priv, INTEL_MEMORY_SYSTEM,
> >                                &args->size, &args->handle);
> >  }
> >
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ramalingam C Dec. 5, 2019, 1:02 p.m. UTC | #4
On 2019-12-05 at 12:20:12 +0000, Chris Wilson wrote:
> Quoting Matthew Auld (2019-12-05 12:12:19)
> > On Mon, 2 Dec 2019 at 06:55, Ramalingam C <ramalingam.c@intel.com> wrote:
> > >
> > > When LMEM is supported, dumb buffer preferred to be created from LMEM.
> > >
> > > v2:
> > >   Parameters are reshuffled. [Chris]
> > > v3:
> > >   s/region_id/mem_type
> > > v4:
> > >   use the i915_gem_object_create_region [chris]
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > cc: Matthew Auld <matthew.auld@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 61395b03443e..34e480c8293e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -44,6 +44,7 @@
> > >  #include "gem/i915_gem_clflush.h"
> > >  #include "gem/i915_gem_context.h"
> > >  #include "gem/i915_gem_ioctls.h"
> > > +#include "gem/i915_gem_region.h"
> > >  #include "gem/i915_gem_pm.h"
> > >  #include "gt/intel_context.h"
> > >  #include "gt/intel_engine_user.h"
> > > @@ -176,6 +177,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> > >  static int
> > >  i915_gem_create(struct drm_file *file,
> > >                 struct drm_i915_private *dev_priv,
> > > +               enum intel_memory_type mem_type,
> > >                 u64 *size_p,
> > >                 u32 *handle_p)
> > >  {
> > > @@ -189,7 +191,8 @@ i915_gem_create(struct drm_file *file,
> > >                 return -EINVAL;
> > >
> > >         /* Allocate the new object */
> > > -       obj = i915_gem_object_create_shmem(dev_priv, size);
> > > +       obj = i915_gem_object_create_region(intel_memory_region_lookup(dev_priv,
> > > +                                           mem_type), size, 0);
> > 
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> > 
> > But how should we go about testing this?
> > 
> > We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> > in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> > that uses get_avail_ram_mb() for always_clear, but maybe we need the
> > query region uapi for that?
> 
> Hmm. Questions over the maximum size for dumb buffer, maximum number of
> dumb buffers, etc, should be addressed to the dumb API. So some form of
> drmGetCap() ?
Chris, Is this suggestion to add this capability probing through a new IOCTL for
dumb APIs? Please clarify.

-Ram
> 
> Note for merging, we need the lmem vm_fault implementation first.
> -Chris
Chris Wilson Dec. 5, 2019, 1:11 p.m. UTC | #5
Quoting Ramalingam C (2019-12-05 13:02:40)
> On 2019-12-05 at 12:20:12 +0000, Chris Wilson wrote:
> > Quoting Matthew Auld (2019-12-05 12:12:19)
> > > We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> > > in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> > > that uses get_avail_ram_mb() for always_clear, but maybe we need the
> > > query region uapi for that?
> > 
> > Hmm. Questions over the maximum size for dumb buffer, maximum number of
> > dumb buffers, etc, should be addressed to the dumb API. So some form of
> > drmGetCap() ?
> Chris, Is this suggestion to add this capability probing through a new IOCTL for
> dumb APIs? Please clarify.

I don't think we need a new ioctl, as drm_getcap already covers the dumb
buffer API. We just need to expose the limits of the dumb buffer API
through it.

The 2 that spring to mind are maximum size of individual buffer and
maximum size of total dumb buffers. (The latter may be infinite for
drivers that allow swapping of inactive buffers.) There was a request on
irc for something like this as well, but I'm not aware of the context.
-Chris
Ramalingam C Dec. 9, 2019, 11:57 a.m. UTC | #6
On 2019-12-05 at 13:11:29 +0000, Chris Wilson wrote:
> Quoting Ramalingam C (2019-12-05 13:02:40)
> > On 2019-12-05 at 12:20:12 +0000, Chris Wilson wrote:
> > > Quoting Matthew Auld (2019-12-05 12:12:19)
> > > > We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> > > > in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> > > > that uses get_avail_ram_mb() for always_clear, but maybe we need the
> > > > query region uapi for that?
> > > 
> > > Hmm. Questions over the maximum size for dumb buffer, maximum number of
> > > dumb buffers, etc, should be addressed to the dumb API. So some form of
> > > drmGetCap() ?
> > Chris, Is this suggestion to add this capability probing through a new IOCTL for
> > dumb APIs? Please clarify.
> 
> I don't think we need a new ioctl, as drm_getcap already covers the dumb
> buffer API. We just need to expose the limits of the dumb buffer API
> through it.
> 
> The 2 that spring to mind are maximum size of individual buffer and
> maximum size of total dumb buffers.

Will there be question for userspace for this extension or no?
AFAIK There is no consumer except IGT.

-Ram

> (The latter may be infinite for
> drivers that allow swapping of inactive buffers.) There was a request on
> irc for something like this as well, but I'm not aware of the context.
> -Chris
Chris Wilson Dec. 9, 2019, 12:03 p.m. UTC | #7
Quoting Ramalingam C (2019-12-09 11:57:01)
> On 2019-12-05 at 13:11:29 +0000, Chris Wilson wrote:
> > Quoting Ramalingam C (2019-12-05 13:02:40)
> > > On 2019-12-05 at 12:20:12 +0000, Chris Wilson wrote:
> > > > Quoting Matthew Auld (2019-12-05 12:12:19)
> > > > > We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> > > > > in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> > > > > that uses get_avail_ram_mb() for always_clear, but maybe we need the
> > > > > query region uapi for that?
> > > > 
> > > > Hmm. Questions over the maximum size for dumb buffer, maximum number of
> > > > dumb buffers, etc, should be addressed to the dumb API. So some form of
> > > > drmGetCap() ?
> > > Chris, Is this suggestion to add this capability probing through a new IOCTL for
> > > dumb APIs? Please clarify.
> > 
> > I don't think we need a new ioctl, as drm_getcap already covers the dumb
> > buffer API. We just need to expose the limits of the dumb buffer API
> > through it.
> > 
> > The 2 that spring to mind are maximum size of individual buffer and
> > maximum size of total dumb buffers.
> 
> Will there be question for userspace for this extension or no?
> AFAIK There is no consumer except IGT.

There was someone else asking for maximum dumb buffer size on irc from
an application perspective.

But I strongly believe that discoverability and testability of an API
should be a central tenet of API design. :)
-Chris
Ramalingam C Dec. 9, 2019, 12:05 p.m. UTC | #8
On 2019-12-09 at 12:03:22 +0000, Chris Wilson wrote:
> Quoting Ramalingam C (2019-12-09 11:57:01)
> > On 2019-12-05 at 13:11:29 +0000, Chris Wilson wrote:
> > > Quoting Ramalingam C (2019-12-05 13:02:40)
> > > > On 2019-12-05 at 12:20:12 +0000, Chris Wilson wrote:
> > > > > Quoting Matthew Auld (2019-12-05 12:12:19)
> > > > > > We would still need to clear the object(maybe I915_BO_ALLOC_CLEARED?)
> > > > > > in order to pass the IGTs. We also need to adjust dumb_buffer.c, since
> > > > > > that uses get_avail_ram_mb() for always_clear, but maybe we need the
> > > > > > query region uapi for that?
> > > > > 
> > > > > Hmm. Questions over the maximum size for dumb buffer, maximum number of
> > > > > dumb buffers, etc, should be addressed to the dumb API. So some form of
> > > > > drmGetCap() ?
> > > > Chris, Is this suggestion to add this capability probing through a new IOCTL for
> > > > dumb APIs? Please clarify.
> > > 
> > > I don't think we need a new ioctl, as drm_getcap already covers the dumb
> > > buffer API. We just need to expose the limits of the dumb buffer API
> > > through it.
> > > 
> > > The 2 that spring to mind are maximum size of individual buffer and
> > > maximum size of total dumb buffers.
> > 
> > Will there be question for userspace for this extension or no?
> > AFAIK There is no consumer except IGT.
> 
> There was someone else asking for maximum dumb buffer size on irc from
> an application perspective.
> 
> But I strongly believe that discoverability and testability of an API
> should be a central tenet of API design. :)
So can i take it for "No userspace is required for this uAPI extension
as this helps the testing" ?

-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 61395b03443e..34e480c8293e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -44,6 +44,7 @@ 
 #include "gem/i915_gem_clflush.h"
 #include "gem/i915_gem_context.h"
 #include "gem/i915_gem_ioctls.h"
+#include "gem/i915_gem_region.h"
 #include "gem/i915_gem_pm.h"
 #include "gt/intel_context.h"
 #include "gt/intel_engine_user.h"
@@ -176,6 +177,7 @@  i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
 static int
 i915_gem_create(struct drm_file *file,
 		struct drm_i915_private *dev_priv,
+		enum intel_memory_type mem_type,
 		u64 *size_p,
 		u32 *handle_p)
 {
@@ -189,7 +191,8 @@  i915_gem_create(struct drm_file *file,
 		return -EINVAL;
 
 	/* Allocate the new object */
-	obj = i915_gem_object_create_shmem(dev_priv, size);
+	obj = i915_gem_object_create_region(intel_memory_region_lookup(dev_priv,
+					    mem_type), size, 0);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
@@ -209,6 +212,7 @@  i915_gem_dumb_create(struct drm_file *file,
 		     struct drm_device *dev,
 		     struct drm_mode_create_dumb *args)
 {
+	enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
 	int cpp = DIV_ROUND_UP(args->bpp, 8);
 	u32 format;
 
@@ -235,7 +239,11 @@  i915_gem_dumb_create(struct drm_file *file,
 		args->pitch = ALIGN(args->pitch, 4096);
 
 	args->size = args->pitch * args->height;
-	return i915_gem_create(file, to_i915(dev),
+
+	if (HAS_LMEM(to_i915(dev)))
+		mem_type = INTEL_MEMORY_LOCAL;
+
+	return i915_gem_create(file, to_i915(dev), mem_type,
 			       &args->size, &args->handle);
 }
 
@@ -254,7 +262,7 @@  i915_gem_create_ioctl(struct drm_device *dev, void *data,
 
 	i915_gem_flush_free_objects(dev_priv);
 
-	return i915_gem_create(file, dev_priv,
+	return i915_gem_create(file, dev_priv, INTEL_MEMORY_SYSTEM,
 			       &args->size, &args->handle);
 }