[v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects
diff mbox series

Message ID 20200528170604.22476-1-rohan.garg@collabora.com
State New
Headers show
Series
  • [v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects
Related show

Commit Message

Rohan Garg May 28, 2020, 5:06 p.m. UTC
DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
with a handle, making it easier to debug issues in userspace
applications.

DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
with a buffer.

Changes in v2:
  - Hoist the IOCTL up into the drm_driver framework

Changes in v3:
  - Introduce a drm_gem_set_label for drivers to use internally
    in order to label a GEM object
  - Hoist string copying up into the IOCTL
  - Fix documentation
  - Move actual gem labeling into drm_gem_adopt_label

Changes in v4:
  - Refactor IOCTL call to only perform string duplication and move
    all gem lookup logic into GEM specific call

Changes in v5:
  - Fix issues pointed out by kbuild test robot
  - Cleanup minor issues around kfree and out/err labels
  - Fixed API documentation issues
  - Rename to DRM_IOCTL_HANDLE_SET_LABEL
  - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
  - Added some documentation for consumers of this IOCTL
  - Ensure that label's cannot be longer than PAGE_SIZE
  - Set a default label value
  - Drop useless warning
  - Properly return length of label to userspace even if
    userspace did not allocate memory for label.

Changes in v6:
  - Wrap code to make better use of 80 char limit
  - Drop redundant copies of the label
  - Protect concurrent access to labels
  - Improved documentation
  - Refactor setter/getter ioctl's to be static
  - Return EINVAL when label length > PAGE_SIZE
  - Simplify code by calling the default GEM label'ing
    function for all drivers using GEM
  - Do not error out when fetching empty labels
  - Refactor flags to the u32 type and add documentation
  - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
  - Return length of copied label to userspace

Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
---
 drivers/gpu/drm/drm_gem.c      | 68 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_internal.h |  5 +++
 drivers/gpu/drm/drm_ioctl.c    | 70 ++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h          | 30 +++++++++++++++
 include/drm/drm_gem.h          | 19 +++++++++
 include/uapi/drm/drm.h         | 25 +++++++++++-
 6 files changed, 216 insertions(+), 1 deletion(-)

Comments

Eric Anholt May 28, 2020, 6:45 p.m. UTC | #1
On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg@collabora.com> wrote:
>
> DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> with a handle, making it easier to debug issues in userspace
> applications.
>
> DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> with a buffer.
>
> Changes in v2:
>   - Hoist the IOCTL up into the drm_driver framework
>
> Changes in v3:
>   - Introduce a drm_gem_set_label for drivers to use internally
>     in order to label a GEM object
>   - Hoist string copying up into the IOCTL
>   - Fix documentation
>   - Move actual gem labeling into drm_gem_adopt_label
>
> Changes in v4:
>   - Refactor IOCTL call to only perform string duplication and move
>     all gem lookup logic into GEM specific call
>
> Changes in v5:
>   - Fix issues pointed out by kbuild test robot
>   - Cleanup minor issues around kfree and out/err labels
>   - Fixed API documentation issues
>   - Rename to DRM_IOCTL_HANDLE_SET_LABEL
>   - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
>   - Added some documentation for consumers of this IOCTL
>   - Ensure that label's cannot be longer than PAGE_SIZE
>   - Set a default label value
>   - Drop useless warning
>   - Properly return length of label to userspace even if
>     userspace did not allocate memory for label.
>
> Changes in v6:
>   - Wrap code to make better use of 80 char limit
>   - Drop redundant copies of the label
>   - Protect concurrent access to labels
>   - Improved documentation
>   - Refactor setter/getter ioctl's to be static
>   - Return EINVAL when label length > PAGE_SIZE
>   - Simplify code by calling the default GEM label'ing
>     function for all drivers using GEM
>   - Do not error out when fetching empty labels
>   - Refactor flags to the u32 type and add documentation
>   - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
>   - Return length of copied label to userspace
>
> Signed-off-by: Rohan Garg <rohan.garg@collabora.com>

I don't think we should land this until it actually does something
with the label, that feels out of the spirit of our uapi merge rules.
I would suggest looking at dma_buf_set_name(), which would produce
useful output in debugfs's /dma_buf/buf_info.  But also presumably you
have something in panfrost using this?

> +int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
> +                     struct drm_handle_label *args)
> +{
> +       struct drm_gem_object *gem_obj;
> +       int len, ret;
> +
> +       gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> +       if (!gem_obj) {
> +               DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
> +               return -ENOENT;
> +       }
> +
> +       if (!gem_obj->label) {
> +               args->label = NULL;
> +               args->len = 0;
> +               return 0;
> +       }
> +
> +       mutex_lock(&gem_obj->bo_lock);
> +       len = strlen(gem_obj->label);
> +       ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
> +                          min(args->len, len));
> +       mutex_unlock(&gem_obj->bo_lock);
> +       args->len = len;
> +       drm_gem_object_put(gem_obj);
> +       return ret;
> +}

I think the !gem_obj->label code path also needs to be under the lock,
otherwise you could be racing to copy_to_user from a NULL pointer,
right?

>  #define DRM_CORE_IOCTL_COUNT   ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index bb924cddc09c..6fcb7b9ff322 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -540,6 +540,36 @@ struct drm_driver {
>                             struct drm_device *dev,
>                             uint32_t handle);
>
> +       /**
> +        * @set_label:
> +        *
> +        * Label a buffer object
> +        *
> +        * Called by the user via ioctl.
> +        *
> +        * Returns:
> +        *
> +        * Length of label on success, negative errno on failure.
> +        */
> +       int (*set_label)(struct drm_device *dev,
> +                        struct drm_file *file_priv,
> +                        struct drm_handle_label *args);
> +
> +       /**
> +        * @get_label:
> +        *
> +        * Read the label of a buffer object.
> +        *
> +        * Called by the user via ioctl.
> +        *
> +        * Returns:
> +        *
> +        * Length of label on success, negative errno on failiure.
> +        */
> +       char *(*get_label)(struct drm_device *dev,
> +                          struct drm_file *file_priv,
> +                          struct drm_handle_label *args);
> +

I think the documentation should note that these are optional.
Rohan Garg May 29, 2020, 1:44 p.m. UTC | #2
Hey Eric!

On jueves, 28 de mayo de 2020 20:45:24 (CEST) Eric Anholt wrote:
> On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg@collabora.com> 
wrote:
> > DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> > with a handle, making it easier to debug issues in userspace
> > applications.
> > 
> > DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> > with a buffer.
> > 
> > Changes in v2:
> >   - Hoist the IOCTL up into the drm_driver framework
> > 
> > Changes in v3:
> >   - Introduce a drm_gem_set_label for drivers to use internally
> >   
> >     in order to label a GEM object
> >   
> >   - Hoist string copying up into the IOCTL
> >   - Fix documentation
> >   - Move actual gem labeling into drm_gem_adopt_label
> > 
> > Changes in v4:
> >   - Refactor IOCTL call to only perform string duplication and move
> >   
> >     all gem lookup logic into GEM specific call
> > 
> > Changes in v5:
> >   - Fix issues pointed out by kbuild test robot
> >   - Cleanup minor issues around kfree and out/err labels
> >   - Fixed API documentation issues
> >   - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> >   - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> >   - Added some documentation for consumers of this IOCTL
> >   - Ensure that label's cannot be longer than PAGE_SIZE
> >   - Set a default label value
> >   - Drop useless warning
> >   - Properly return length of label to userspace even if
> >   
> >     userspace did not allocate memory for label.
> > 
> > Changes in v6:
> >   - Wrap code to make better use of 80 char limit
> >   - Drop redundant copies of the label
> >   - Protect concurrent access to labels
> >   - Improved documentation
> >   - Refactor setter/getter ioctl's to be static
> >   - Return EINVAL when label length > PAGE_SIZE
> >   - Simplify code by calling the default GEM label'ing
> >   
> >     function for all drivers using GEM
> >   
> >   - Do not error out when fetching empty labels
> >   - Refactor flags to the u32 type and add documentation
> >   - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> >   - Return length of copied label to userspace
> > 
> > Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> 
> I don't think we should land this until it actually does something
> with the label, that feels out of the spirit of our uapi merge rules.
> I would suggest looking at dma_buf_set_name(), which would produce
> useful output in debugfs's /dma_buf/buf_info.  But also presumably you
> have something in panfrost using this?
> 

My current short term plan is to hook up glLabel to the labeling functionality 
in order for userspace applications to debug exactly which buffer objects 
could be causing faults in the kernel for speedier debugging.

The more long term plan is to label each buffer with a unique id that we can 
correlate to the GL calls being flushed in order to be able to profile (a set 
of)  GL calls on various platforms in order to aid driver developers with 
performance work. This could be something that we corelate on the userspace 
side with the help of perfetto by using this [1] patch that emits ftrace 
events.

> > +int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
> > +                     struct drm_handle_label *args)
> > +{
> > +       struct drm_gem_object *gem_obj;
> > +       int len, ret;
> > +
> > +       gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> > +       if (!gem_obj) {
> > +               DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
> > +               return -ENOENT;
> > +       }
> > +
> > +       if (!gem_obj->label) {
> > +               args->label = NULL;
> > +               args->len = 0;
> > +               return 0;
> > +       }
> > +
> > +       mutex_lock(&gem_obj->bo_lock);
> > +       len = strlen(gem_obj->label);
> > +       ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
> > +                          min(args->len, len));
> > +       mutex_unlock(&gem_obj->bo_lock);
> > +       args->len = len;
> > +       drm_gem_object_put(gem_obj);
> > +       return ret;
> > +}
> 
> I think the !gem_obj->label code path also needs to be under the lock,
> otherwise you could be racing to copy_to_user from a NULL pointer,
> right?
> 

You're absolutely correct.

> >  #define DRM_CORE_IOCTL_COUNT   ARRAY_SIZE( drm_ioctls )
> > 
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index bb924cddc09c..6fcb7b9ff322 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -540,6 +540,36 @@ struct drm_driver {
> > 
> >                             struct drm_device *dev,
> >                             uint32_t handle);
> > 
> > +       /**
> > +        * @set_label:
> > +        *
> > +        * Label a buffer object
> > +        *
> > +        * Called by the user via ioctl.
> > +        *
> > +        * Returns:
> > +        *
> > +        * Length of label on success, negative errno on failure.
> > +        */
> > +       int (*set_label)(struct drm_device *dev,
> > +                        struct drm_file *file_priv,
> > +                        struct drm_handle_label *args);
> > +
> > +       /**
> > +        * @get_label:
> > +        *
> > +        * Read the label of a buffer object.
> > +        *
> > +        * Called by the user via ioctl.
> > +        *
> > +        * Returns:
> > +        *
> > +        * Length of label on success, negative errno on failiure.
> > +        */
> > +       char *(*get_label)(struct drm_device *dev,
> > +                          struct drm_file *file_priv,
> > +                          struct drm_handle_label *args);
> > +
> 
> I think the documentation should note that these are optional.

Gotcha.

Thanks!
Rohan Garg

[1] https://gitlab.freedesktop.org/shadeslayer/linux/-/commit/
bc9625b0f73f7ccdb04f9cf3bf6c5a609e3bbcbd
Eric Anholt May 29, 2020, 5:10 p.m. UTC | #3
On Fri, May 29, 2020 at 6:44 AM Rohan Garg <rohan.garg@collabora.com> wrote:
>
> Hey Eric!
>
> On jueves, 28 de mayo de 2020 20:45:24 (CEST) Eric Anholt wrote:
> > On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg@collabora.com>
> wrote:
> > > DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> > > with a handle, making it easier to debug issues in userspace
> > > applications.
> > >
> > > DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> > > with a buffer.
> > >
> > > Changes in v2:
> > >   - Hoist the IOCTL up into the drm_driver framework
> > >
> > > Changes in v3:
> > >   - Introduce a drm_gem_set_label for drivers to use internally
> > >
> > >     in order to label a GEM object
> > >
> > >   - Hoist string copying up into the IOCTL
> > >   - Fix documentation
> > >   - Move actual gem labeling into drm_gem_adopt_label
> > >
> > > Changes in v4:
> > >   - Refactor IOCTL call to only perform string duplication and move
> > >
> > >     all gem lookup logic into GEM specific call
> > >
> > > Changes in v5:
> > >   - Fix issues pointed out by kbuild test robot
> > >   - Cleanup minor issues around kfree and out/err labels
> > >   - Fixed API documentation issues
> > >   - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> > >   - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> > >   - Added some documentation for consumers of this IOCTL
> > >   - Ensure that label's cannot be longer than PAGE_SIZE
> > >   - Set a default label value
> > >   - Drop useless warning
> > >   - Properly return length of label to userspace even if
> > >
> > >     userspace did not allocate memory for label.
> > >
> > > Changes in v6:
> > >   - Wrap code to make better use of 80 char limit
> > >   - Drop redundant copies of the label
> > >   - Protect concurrent access to labels
> > >   - Improved documentation
> > >   - Refactor setter/getter ioctl's to be static
> > >   - Return EINVAL when label length > PAGE_SIZE
> > >   - Simplify code by calling the default GEM label'ing
> > >
> > >     function for all drivers using GEM
> > >
> > >   - Do not error out when fetching empty labels
> > >   - Refactor flags to the u32 type and add documentation
> > >   - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> > >   - Return length of copied label to userspace
> > >
> > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> >
> > I don't think we should land this until it actually does something
> > with the label, that feels out of the spirit of our uapi merge rules.
> > I would suggest looking at dma_buf_set_name(), which would produce
> > useful output in debugfs's /dma_buf/buf_info.  But also presumably you
> > have something in panfrost using this?
> >
>
> My current short term plan is to hook up glLabel to the labeling functionality
> in order for userspace applications to debug exactly which buffer objects
> could be causing faults in the kernel for speedier debugging.

So, something like "when a fault happens, dump/capture names of nearby
objects"?  That would certainly be nice to have!

> The more long term plan is to label each buffer with a unique id that we can
> correlate to the GL calls being flushed in order to be able to profile (a set
> of)  GL calls on various platforms in order to aid driver developers with
> performance work. This could be something that we corelate on the userspace
> side with the help of perfetto by using this [1] patch that emits ftrace
> events.

I'm curious how BO names are part of profiling?  Do you have the
ability to sample instruction IP and use that to figure out where time
is spent in shaders, or something?
kernel test robot May 30, 2020, 6:16 p.m. UTC | #4
Hi Rohan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-set-and-get-a-label-on-GEM-objects/20200531-000134
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from drivers/gpu/drm/drm_gem_vram_helper.c:7:
>> include/drm/drm_drv.h:566:12: warning: 'struct drm_handle_label' declared inside parameter list will not be visible outside of this definition or declaration
566 |     struct drm_handle_label *args);
|            ^~~~~~~~~~~~~~~~
include/drm/drm_drv.h:581:14: warning: 'struct drm_handle_label' declared inside parameter list will not be visible outside of this definition or declaration
581 |       struct drm_handle_label *args);
|              ^~~~~~~~~~~~~~~~
--
drivers/gpu/drm/drm_gem.c: In function 'drm_gem_set_label':
drivers/gpu/drm/drm_gem.c:949:14: warning: unused variable '_pid' [-Wunused-variable]
949 |  struct pid *_pid = get_task_pid(current, PIDTYPE_PID);
|              ^~~~
drivers/gpu/drm/drm_gem.c: In function 'drm_gem_adopt_label':
>> drivers/gpu/drm/drm_gem.c:974:17: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
974 |  gem_obj->label = label;
|                 ^
drivers/gpu/drm/drm_gem.c: In function 'drm_gem_get_label':
>> drivers/gpu/drm/drm_gem.c:992:15: warning: assignment to '__u64' {aka 'long long unsigned int'} from 'void *' makes integer from pointer without a cast [-Wint-conversion]
992 |   args->label = NULL;
|               ^
In file included from include/asm-generic/bug.h:19,
from arch/ia64/include/asm/bug.h:17,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from include/linux/slab.h:15,
from drivers/gpu/drm/drm_gem.c:29:
include/linux/kernel.h:842:29: warning: comparison of distinct pointer types lacks a cast
842 |   (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
|                             ^~
include/linux/kernel.h:856:4: note: in expansion of macro '__typecheck'
856 |   (__typecheck(x, y) && __no_side_effects(x, y))
|    ^~~~~~~~~~~
include/linux/kernel.h:866:24: note: in expansion of macro '__safe_cmp'
866 |  __builtin_choose_expr(__safe_cmp(x, y),          |                        ^~~~~~~~~~
include/linux/kernel.h:875:19: note: in expansion of macro '__careful_cmp'
875 | #define min(x, y) __careful_cmp(x, y, <)
|                   ^~~~~~~~~~~~~
>> drivers/gpu/drm/drm_gem.c:1000:7: note: in expansion of macro 'min'
1000 |       min(args->len, len));
|       ^~~
--
drivers/gpu/drm/drm_ioctl.c: In function 'drm_handle_set_label_ioctl':
drivers/gpu/drm/drm_ioctl.c:595:8: warning: unused variable 'label' [-Wunused-variable]
595 |  char *label;
|        ^~~~~
drivers/gpu/drm/drm_ioctl.c: In function 'drm_handle_get_label_ioctl':
>> drivers/gpu/drm/drm_ioctl.c:636:10: warning: returning 'char *' from a function with return type 'int' makes integer from pointer without a cast [-Wint-conversion]
636 |   return dev->driver->get_label(dev, file_priv, args);
|          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/drm_ioctl.c:640:1: warning: control reaches end of non-void function [-Wreturn-type]
640 | }
| ^
drivers/gpu/drm/drm_ioctl.c: In function 'drm_handle_set_label_ioctl':
drivers/gpu/drm/drm_ioctl.c:609:1: warning: control reaches end of non-void function [-Wreturn-type]
609 | }
| ^
--
In file included from drivers/gpu/drm/mgag200/mgag200_drv.c:13:
>> include/drm/drm_drv.h:566:12: warning: 'struct drm_handle_label' declared inside parameter list will not be visible outside of this definition or declaration
566 |     struct drm_handle_label *args);
|            ^~~~~~~~~~~~~~~~
include/drm/drm_drv.h:581:14: warning: 'struct drm_handle_label' declared inside parameter list will not be visible outside of this definition or declaration
581 |       struct drm_handle_label *args);
|              ^~~~~~~~~~~~~~~~
drivers/gpu/drm/mgag200/mgag200_drv.c:119:5: warning: no previous prototype for 'mgag200_driver_dumb_create' [-Wmissing-prototypes]
119 | int mgag200_driver_dumb_create(struct drm_file *file,
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~

vim +566 include/drm/drm_drv.h

   152	
   153	/**
   154	 * struct drm_driver - DRM driver structure
   155	 *
   156	 * This structure represent the common code for a family of cards. There will be
   157	 * one &struct drm_device for each card present in this family. It contains lots
   158	 * of vfunc entries, and a pile of those probably should be moved to more
   159	 * appropriate places like &drm_mode_config_funcs or into a new operations
   160	 * structure for GEM drivers.
   161	 */
   162	struct drm_driver {
   163		/**
   164		 * @load:
   165		 *
   166		 * Backward-compatible driver callback to complete
   167		 * initialization steps after the driver is registered.  For
   168		 * this reason, may suffer from race conditions and its use is
   169		 * deprecated for new drivers.  It is therefore only supported
   170		 * for existing drivers not yet converted to the new scheme.
   171		 * See drm_dev_init() and drm_dev_register() for proper and
   172		 * race-free way to set up a &struct drm_device.
   173		 *
   174		 * This is deprecated, do not use!
   175		 *
   176		 * Returns:
   177		 *
   178		 * Zero on success, non-zero value on failure.
   179		 */
   180		int (*load) (struct drm_device *, unsigned long flags);
   181	
   182		/**
   183		 * @open:
   184		 *
   185		 * Driver callback when a new &struct drm_file is opened. Useful for
   186		 * setting up driver-private data structures like buffer allocators,
   187		 * execution contexts or similar things. Such driver-private resources
   188		 * must be released again in @postclose.
   189		 *
   190		 * Since the display/modeset side of DRM can only be owned by exactly
   191		 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
   192		 * there should never be a need to set up any modeset related resources
   193		 * in this callback. Doing so would be a driver design bug.
   194		 *
   195		 * Returns:
   196		 *
   197		 * 0 on success, a negative error code on failure, which will be
   198		 * promoted to userspace as the result of the open() system call.
   199		 */
   200		int (*open) (struct drm_device *, struct drm_file *);
   201	
   202		/**
   203		 * @postclose:
   204		 *
   205		 * One of the driver callbacks when a new &struct drm_file is closed.
   206		 * Useful for tearing down driver-private data structures allocated in
   207		 * @open like buffer allocators, execution contexts or similar things.
   208		 *
   209		 * Since the display/modeset side of DRM can only be owned by exactly
   210		 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
   211		 * there should never be a need to tear down any modeset related
   212		 * resources in this callback. Doing so would be a driver design bug.
   213		 */
   214		void (*postclose) (struct drm_device *, struct drm_file *);
   215	
   216		/**
   217		 * @lastclose:
   218		 *
   219		 * Called when the last &struct drm_file has been closed and there's
   220		 * currently no userspace client for the &struct drm_device.
   221		 *
   222		 * Modern drivers should only use this to force-restore the fbdev
   223		 * framebuffer using drm_fb_helper_restore_fbdev_mode_unlocked().
   224		 * Anything else would indicate there's something seriously wrong.
   225		 * Modern drivers can also use this to execute delayed power switching
   226		 * state changes, e.g. in conjunction with the :ref:`vga_switcheroo`
   227		 * infrastructure.
   228		 *
   229		 * This is called after @postclose hook has been called.
   230		 *
   231		 * NOTE:
   232		 *
   233		 * All legacy drivers use this callback to de-initialize the hardware.
   234		 * This is purely because of the shadow-attach model, where the DRM
   235		 * kernel driver does not really own the hardware. Instead ownershipe is
   236		 * handled with the help of userspace through an inheritedly racy dance
   237		 * to set/unset the VT into raw mode.
   238		 *
   239		 * Legacy drivers initialize the hardware in the @firstopen callback,
   240		 * which isn't even called for modern drivers.
   241		 */
   242		void (*lastclose) (struct drm_device *);
   243	
   244		/**
   245		 * @unload:
   246		 *
   247		 * Reverse the effects of the driver load callback.  Ideally,
   248		 * the clean up performed by the driver should happen in the
   249		 * reverse order of the initialization.  Similarly to the load
   250		 * hook, this handler is deprecated and its usage should be
   251		 * dropped in favor of an open-coded teardown function at the
   252		 * driver layer.  See drm_dev_unregister() and drm_dev_put()
   253		 * for the proper way to remove a &struct drm_device.
   254		 *
   255		 * The unload() hook is called right after unregistering
   256		 * the device.
   257		 *
   258		 */
   259		void (*unload) (struct drm_device *);
   260	
   261		/**
   262		 * @release:
   263		 *
   264		 * Optional callback for destroying device data after the final
   265		 * reference is released, i.e. the device is being destroyed.
   266		 *
   267		 * This is deprecated, clean up all memory allocations associated with a
   268		 * &drm_device using drmm_add_action(), drmm_kmalloc() and related
   269		 * managed resources functions.
   270		 */
   271		void (*release) (struct drm_device *);
   272	
   273		/**
   274		 * @irq_handler:
   275		 *
   276		 * Interrupt handler called when using drm_irq_install(). Not used by
   277		 * drivers which implement their own interrupt handling.
   278		 */
   279		irqreturn_t(*irq_handler) (int irq, void *arg);
   280	
   281		/**
   282		 * @irq_preinstall:
   283		 *
   284		 * Optional callback used by drm_irq_install() which is called before
   285		 * the interrupt handler is registered. This should be used to clear out
   286		 * any pending interrupts (from e.g. firmware based drives) and reset
   287		 * the interrupt handling registers.
   288		 */
   289		void (*irq_preinstall) (struct drm_device *dev);
   290	
   291		/**
   292		 * @irq_postinstall:
   293		 *
   294		 * Optional callback used by drm_irq_install() which is called after
   295		 * the interrupt handler is registered. This should be used to enable
   296		 * interrupt generation in the hardware.
   297		 */
   298		int (*irq_postinstall) (struct drm_device *dev);
   299	
   300		/**
   301		 * @irq_uninstall:
   302		 *
   303		 * Optional callback used by drm_irq_uninstall() which is called before
   304		 * the interrupt handler is unregistered. This should be used to disable
   305		 * interrupt generation in the hardware.
   306		 */
   307		void (*irq_uninstall) (struct drm_device *dev);
   308	
   309		/**
   310		 * @master_set:
   311		 *
   312		 * Called whenever the minor master is set. Only used by vmwgfx.
   313		 */
   314		int (*master_set)(struct drm_device *dev, struct drm_file *file_priv,
   315				  bool from_open);
   316		/**
   317		 * @master_drop:
   318		 *
   319		 * Called whenever the minor master is dropped. Only used by vmwgfx.
   320		 */
   321		void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv);
   322	
   323		/**
   324		 * @debugfs_init:
   325		 *
   326		 * Allows drivers to create driver-specific debugfs files.
   327		 */
   328		void (*debugfs_init)(struct drm_minor *minor);
   329	
   330		/**
   331		 * @gem_free_object: deconstructor for drm_gem_objects
   332		 *
   333		 * This is deprecated and should not be used by new drivers. Use
   334		 * &drm_gem_object_funcs.free instead.
   335		 */
   336		void (*gem_free_object) (struct drm_gem_object *obj);
   337	
   338		/**
   339		 * @gem_free_object_unlocked: deconstructor for drm_gem_objects
   340		 *
   341		 * This is deprecated and should not be used by new drivers. Use
   342		 * &drm_gem_object_funcs.free instead.
   343		 * Compared to @gem_free_object this is not encumbered with
   344		 * &drm_device.struct_mutex legacy locking schemes.
   345		 */
   346		void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
   347	
   348		/**
   349		 * @gem_open_object:
   350		 *
   351		 * This callback is deprecated in favour of &drm_gem_object_funcs.open.
   352		 *
   353		 * Driver hook called upon gem handle creation
   354		 */
   355		int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
   356	
   357		/**
   358		 * @gem_close_object:
   359		 *
   360		 * This callback is deprecated in favour of &drm_gem_object_funcs.close.
   361		 *
   362		 * Driver hook called upon gem handle release
   363		 */
   364		void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
   365	
   366		/**
   367		 * @gem_print_info:
   368		 *
   369		 * This callback is deprecated in favour of
   370		 * &drm_gem_object_funcs.print_info.
   371		 *
   372		 * If driver subclasses struct &drm_gem_object, it can implement this
   373		 * optional hook for printing additional driver specific info.
   374		 *
   375		 * drm_printf_indent() should be used in the callback passing it the
   376		 * indent argument.
   377		 *
   378		 * This callback is called from drm_gem_print_info().
   379		 */
   380		void (*gem_print_info)(struct drm_printer *p, unsigned int indent,
   381				       const struct drm_gem_object *obj);
   382	
   383		/**
   384		 * @gem_create_object: constructor for gem objects
   385		 *
   386		 * Hook for allocating the GEM object struct, for use by the CMA and
   387		 * SHMEM GEM helpers.
   388		 */
   389		struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
   390							    size_t size);
   391		/**
   392		 * @prime_handle_to_fd:
   393		 *
   394		 * Main PRIME export function. Should be implemented with
   395		 * drm_gem_prime_handle_to_fd() for GEM based drivers.
   396		 *
   397		 * For an in-depth discussion see :ref:`PRIME buffer sharing
   398		 * documentation <prime_buffer_sharing>`.
   399		 */
   400		int (*prime_handle_to_fd)(struct drm_device *dev, struct drm_file *file_priv,
   401					uint32_t handle, uint32_t flags, int *prime_fd);
   402		/**
   403		 * @prime_fd_to_handle:
   404		 *
   405		 * Main PRIME import function. Should be implemented with
   406		 * drm_gem_prime_fd_to_handle() for GEM based drivers.
   407		 *
   408		 * For an in-depth discussion see :ref:`PRIME buffer sharing
   409		 * documentation <prime_buffer_sharing>`.
   410		 */
   411		int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file *file_priv,
   412					int prime_fd, uint32_t *handle);
   413		/**
   414		 * @gem_prime_export:
   415		 *
   416		 * Export hook for GEM drivers. Deprecated in favour of
   417		 * &drm_gem_object_funcs.export.
   418		 */
   419		struct dma_buf * (*gem_prime_export)(struct drm_gem_object *obj,
   420						     int flags);
   421		/**
   422		 * @gem_prime_import:
   423		 *
   424		 * Import hook for GEM drivers.
   425		 *
   426		 * This defaults to drm_gem_prime_import() if not set.
   427		 */
   428		struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
   429					struct dma_buf *dma_buf);
   430	
   431		/**
   432		 * @gem_prime_pin:
   433		 *
   434		 * Deprecated hook in favour of &drm_gem_object_funcs.pin.
   435		 */
   436		int (*gem_prime_pin)(struct drm_gem_object *obj);
   437	
   438		/**
   439		 * @gem_prime_unpin:
   440		 *
   441		 * Deprecated hook in favour of &drm_gem_object_funcs.unpin.
   442		 */
   443		void (*gem_prime_unpin)(struct drm_gem_object *obj);
   444	
   445	
   446		/**
   447		 * @gem_prime_get_sg_table:
   448		 *
   449		 * Deprecated hook in favour of &drm_gem_object_funcs.get_sg_table.
   450		 */
   451		struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
   452	
   453		/**
   454		 * @gem_prime_import_sg_table:
   455		 *
   456		 * Optional hook used by the PRIME helper functions
   457		 * drm_gem_prime_import() respectively drm_gem_prime_import_dev().
   458		 */
   459		struct drm_gem_object *(*gem_prime_import_sg_table)(
   460					struct drm_device *dev,
   461					struct dma_buf_attachment *attach,
   462					struct sg_table *sgt);
   463		/**
   464		 * @gem_prime_vmap:
   465		 *
   466		 * Deprecated vmap hook for GEM drivers. Please use
   467		 * &drm_gem_object_funcs.vmap instead.
   468		 */
   469		void *(*gem_prime_vmap)(struct drm_gem_object *obj);
   470	
   471		/**
   472		 * @gem_prime_vunmap:
   473		 *
   474		 * Deprecated vunmap hook for GEM drivers. Please use
   475		 * &drm_gem_object_funcs.vunmap instead.
   476		 */
   477		void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
   478	
   479		/**
   480		 * @gem_prime_mmap:
   481		 *
   482		 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
   483		 * PRIME helpers.
   484		 *
   485		 * FIXME: There's way too much duplication going on here, and also moved
   486		 * to &drm_gem_object_funcs.
   487		 */
   488		int (*gem_prime_mmap)(struct drm_gem_object *obj,
   489					struct vm_area_struct *vma);
   490	
   491		/**
   492		 * @dumb_create:
   493		 *
   494		 * This creates a new dumb buffer in the driver's backing storage manager (GEM,
   495		 * TTM or something else entirely) and returns the resulting buffer handle. This
   496		 * handle can then be wrapped up into a framebuffer modeset object.
   497		 *
   498		 * Note that userspace is not allowed to use such objects for render
   499		 * acceleration - drivers must create their own private ioctls for such a use
   500		 * case.
   501		 *
   502		 * Width, height and depth are specified in the &drm_mode_create_dumb
   503		 * argument. The callback needs to fill the handle, pitch and size for
   504		 * the created buffer.
   505		 *
   506		 * Called by the user via ioctl.
   507		 *
   508		 * Returns:
   509		 *
   510		 * Zero on success, negative errno on failure.
   511		 */
   512		int (*dumb_create)(struct drm_file *file_priv,
   513				   struct drm_device *dev,
   514				   struct drm_mode_create_dumb *args);
   515		/**
   516		 * @dumb_map_offset:
   517		 *
   518		 * Allocate an offset in the drm device node's address space to be able to
   519		 * memory map a dumb buffer.
   520		 *
   521		 * The default implementation is drm_gem_create_mmap_offset(). GEM based
   522		 * drivers must not overwrite this.
   523		 *
   524		 * Called by the user via ioctl.
   525		 *
   526		 * Returns:
   527		 *
   528		 * Zero on success, negative errno on failure.
   529		 */
   530		int (*dumb_map_offset)(struct drm_file *file_priv,
   531				       struct drm_device *dev, uint32_t handle,
   532				       uint64_t *offset);
   533		/**
   534		 * @dumb_destroy:
   535		 *
   536		 * This destroys the userspace handle for the given dumb backing storage buffer.
   537		 * Since buffer objects must be reference counted in the kernel a buffer object
   538		 * won't be immediately freed if a framebuffer modeset object still uses it.
   539		 *
   540		 * Called by the user via ioctl.
   541		 *
   542		 * The default implementation is drm_gem_dumb_destroy(). GEM based drivers
   543		 * must not overwrite this.
   544		 *
   545		 * Returns:
   546		 *
   547		 * Zero on success, negative errno on failure.
   548		 */
   549		int (*dumb_destroy)(struct drm_file *file_priv,
   550				    struct drm_device *dev,
   551				    uint32_t handle);
   552	
   553		/**
   554		 * @set_label:
   555		 *
   556		 * Label a buffer object
   557		 *
   558		 * Called by the user via ioctl.
   559		 *
   560		 * Returns:
   561		 *
   562		 * Length of label on success, negative errno on failure.
   563		 */
   564		int (*set_label)(struct drm_device *dev,
   565				 struct drm_file *file_priv,
 > 566				 struct drm_handle_label *args);
   567	
   568		/**
   569		 * @get_label:
   570		 *
   571		 * Read the label of a buffer object.
   572		 *
   573		 * Called by the user via ioctl.
   574		 *
   575		 * Returns:
   576		 *
   577		 * Length of label on success, negative errno on failiure.
   578		 */
   579		char *(*get_label)(struct drm_device *dev,
   580				   struct drm_file *file_priv,
   581				   struct drm_handle_label *args);
   582	
   583		/**
   584		 * @gem_vm_ops: Driver private ops for this object
   585		 *
   586		 * For GEM drivers this is deprecated in favour of
   587		 * &drm_gem_object_funcs.vm_ops.
   588		 */
   589		const struct vm_operations_struct *gem_vm_ops;
   590	
   591		/** @major: driver major number */
   592		int major;
   593		/** @minor: driver minor number */
   594		int minor;
   595		/** @patchlevel: driver patch level */
   596		int patchlevel;
   597		/** @name: driver name */
   598		char *name;
   599		/** @desc: driver description */
   600		char *desc;
   601		/** @date: driver date */
   602		char *date;
   603	
   604		/**
   605		 * @driver_features:
   606		 * Driver features, see &enum drm_driver_feature. Drivers can disable
   607		 * some features on a per-instance basis using
   608		 * &drm_device.driver_features.
   609		 */
   610		u32 driver_features;
   611	
   612		/**
   613		 * @ioctls:
   614		 *
   615		 * Array of driver-private IOCTL description entries. See the chapter on
   616		 * :ref:`IOCTL support in the userland interfaces
   617		 * chapter<drm_driver_ioctl>` for the full details.
   618		 */
   619	
   620		const struct drm_ioctl_desc *ioctls;
   621		/** @num_ioctls: Number of entries in @ioctls. */
   622		int num_ioctls;
   623	
   624		/**
   625		 * @fops:
   626		 *
   627		 * File operations for the DRM device node. See the discussion in
   628		 * :ref:`file operations<drm_driver_fops>` for in-depth coverage and
   629		 * some examples.
   630		 */
   631		const struct file_operations *fops;
   632	
   633		/* Everything below here is for legacy driver, never use! */
   634		/* private: */
   635	
   636		/* List of devices hanging off this driver with stealth attach. */
   637		struct list_head legacy_dev_list;
   638		int (*firstopen) (struct drm_device *);
   639		void (*preclose) (struct drm_device *, struct drm_file *file_priv);
   640		int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
   641		int (*dma_quiescent) (struct drm_device *);
   642		int (*context_dtor) (struct drm_device *dev, int context);
   643		u32 (*get_vblank_counter)(struct drm_device *dev, unsigned int pipe);
   644		int (*enable_vblank)(struct drm_device *dev, unsigned int pipe);
   645		void (*disable_vblank)(struct drm_device *dev, unsigned int pipe);
   646		int dev_priv_size;
   647	};
   648	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot May 31, 2020, 1:56 a.m. UTC | #5
Hi Rohan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-set-and-get-a-label-on-GEM-objects/20200531-000134
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: x86_64-randconfig-a013-20200531 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from drivers/gpu/drm/drm_auth.c:34:0:
>> include/drm/drm_drv.h:566:12: warning: 'struct drm_handle_label' declared inside parameter list
struct drm_handle_label *args);
^
>> include/drm/drm_drv.h:566:12: warning: its scope is only this definition or declaration, which is probably not what you want
include/drm/drm_drv.h:581:14: warning: 'struct drm_handle_label' declared inside parameter list
struct drm_handle_label *args);
^

vim +566 include/drm/drm_drv.h

   152	
   153	/**
   154	 * struct drm_driver - DRM driver structure
   155	 *
   156	 * This structure represent the common code for a family of cards. There will be
   157	 * one &struct drm_device for each card present in this family. It contains lots
   158	 * of vfunc entries, and a pile of those probably should be moved to more
   159	 * appropriate places like &drm_mode_config_funcs or into a new operations
   160	 * structure for GEM drivers.
   161	 */
   162	struct drm_driver {
   163		/**
   164		 * @load:
   165		 *
   166		 * Backward-compatible driver callback to complete
   167		 * initialization steps after the driver is registered.  For
   168		 * this reason, may suffer from race conditions and its use is
   169		 * deprecated for new drivers.  It is therefore only supported
   170		 * for existing drivers not yet converted to the new scheme.
   171		 * See drm_dev_init() and drm_dev_register() for proper and
   172		 * race-free way to set up a &struct drm_device.
   173		 *
   174		 * This is deprecated, do not use!
   175		 *
   176		 * Returns:
   177		 *
   178		 * Zero on success, non-zero value on failure.
   179		 */
   180		int (*load) (struct drm_device *, unsigned long flags);
   181	
   182		/**
   183		 * @open:
   184		 *
   185		 * Driver callback when a new &struct drm_file is opened. Useful for
   186		 * setting up driver-private data structures like buffer allocators,
   187		 * execution contexts or similar things. Such driver-private resources
   188		 * must be released again in @postclose.
   189		 *
   190		 * Since the display/modeset side of DRM can only be owned by exactly
   191		 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
   192		 * there should never be a need to set up any modeset related resources
   193		 * in this callback. Doing so would be a driver design bug.
   194		 *
   195		 * Returns:
   196		 *
   197		 * 0 on success, a negative error code on failure, which will be
   198		 * promoted to userspace as the result of the open() system call.
   199		 */
   200		int (*open) (struct drm_device *, struct drm_file *);
   201	
   202		/**
   203		 * @postclose:
   204		 *
   205		 * One of the driver callbacks when a new &struct drm_file is closed.
   206		 * Useful for tearing down driver-private data structures allocated in
   207		 * @open like buffer allocators, execution contexts or similar things.
   208		 *
   209		 * Since the display/modeset side of DRM can only be owned by exactly
   210		 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
   211		 * there should never be a need to tear down any modeset related
   212		 * resources in this callback. Doing so would be a driver design bug.
   213		 */
   214		void (*postclose) (struct drm_device *, struct drm_file *);
   215	
   216		/**
   217		 * @lastclose:
   218		 *
   219		 * Called when the last &struct drm_file has been closed and there's
   220		 * currently no userspace client for the &struct drm_device.
   221		 *
   222		 * Modern drivers should only use this to force-restore the fbdev
   223		 * framebuffer using drm_fb_helper_restore_fbdev_mode_unlocked().
   224		 * Anything else would indicate there's something seriously wrong.
   225		 * Modern drivers can also use this to execute delayed power switching
   226		 * state changes, e.g. in conjunction with the :ref:`vga_switcheroo`
   227		 * infrastructure.
   228		 *
   229		 * This is called after @postclose hook has been called.
   230		 *
   231		 * NOTE:
   232		 *
   233		 * All legacy drivers use this callback to de-initialize the hardware.
   234		 * This is purely because of the shadow-attach model, where the DRM
   235		 * kernel driver does not really own the hardware. Instead ownershipe is
   236		 * handled with the help of userspace through an inheritedly racy dance
   237		 * to set/unset the VT into raw mode.
   238		 *
   239		 * Legacy drivers initialize the hardware in the @firstopen callback,
   240		 * which isn't even called for modern drivers.
   241		 */
   242		void (*lastclose) (struct drm_device *);
   243	
   244		/**
   245		 * @unload:
   246		 *
   247		 * Reverse the effects of the driver load callback.  Ideally,
   248		 * the clean up performed by the driver should happen in the
   249		 * reverse order of the initialization.  Similarly to the load
   250		 * hook, this handler is deprecated and its usage should be
   251		 * dropped in favor of an open-coded teardown function at the
   252		 * driver layer.  See drm_dev_unregister() and drm_dev_put()
   253		 * for the proper way to remove a &struct drm_device.
   254		 *
   255		 * The unload() hook is called right after unregistering
   256		 * the device.
   257		 *
   258		 */
   259		void (*unload) (struct drm_device *);
   260	
   261		/**
   262		 * @release:
   263		 *
   264		 * Optional callback for destroying device data after the final
   265		 * reference is released, i.e. the device is being destroyed.
   266		 *
   267		 * This is deprecated, clean up all memory allocations associated with a
   268		 * &drm_device using drmm_add_action(), drmm_kmalloc() and related
   269		 * managed resources functions.
   270		 */
   271		void (*release) (struct drm_device *);
   272	
   273		/**
   274		 * @irq_handler:
   275		 *
   276		 * Interrupt handler called when using drm_irq_install(). Not used by
   277		 * drivers which implement their own interrupt handling.
   278		 */
   279		irqreturn_t(*irq_handler) (int irq, void *arg);
   280	
   281		/**
   282		 * @irq_preinstall:
   283		 *
   284		 * Optional callback used by drm_irq_install() which is called before
   285		 * the interrupt handler is registered. This should be used to clear out
   286		 * any pending interrupts (from e.g. firmware based drives) and reset
   287		 * the interrupt handling registers.
   288		 */
   289		void (*irq_preinstall) (struct drm_device *dev);
   290	
   291		/**
   292		 * @irq_postinstall:
   293		 *
   294		 * Optional callback used by drm_irq_install() which is called after
   295		 * the interrupt handler is registered. This should be used to enable
   296		 * interrupt generation in the hardware.
   297		 */
   298		int (*irq_postinstall) (struct drm_device *dev);
   299	
   300		/**
   301		 * @irq_uninstall:
   302		 *
   303		 * Optional callback used by drm_irq_uninstall() which is called before
   304		 * the interrupt handler is unregistered. This should be used to disable
   305		 * interrupt generation in the hardware.
   306		 */
   307		void (*irq_uninstall) (struct drm_device *dev);
   308	
   309		/**
   310		 * @master_set:
   311		 *
   312		 * Called whenever the minor master is set. Only used by vmwgfx.
   313		 */
   314		int (*master_set)(struct drm_device *dev, struct drm_file *file_priv,
   315				  bool from_open);
   316		/**
   317		 * @master_drop:
   318		 *
   319		 * Called whenever the minor master is dropped. Only used by vmwgfx.
   320		 */
   321		void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv);
   322	
   323		/**
   324		 * @debugfs_init:
   325		 *
   326		 * Allows drivers to create driver-specific debugfs files.
   327		 */
   328		void (*debugfs_init)(struct drm_minor *minor);
   329	
   330		/**
   331		 * @gem_free_object: deconstructor for drm_gem_objects
   332		 *
   333		 * This is deprecated and should not be used by new drivers. Use
   334		 * &drm_gem_object_funcs.free instead.
   335		 */
   336		void (*gem_free_object) (struct drm_gem_object *obj);
   337	
   338		/**
   339		 * @gem_free_object_unlocked: deconstructor for drm_gem_objects
   340		 *
   341		 * This is deprecated and should not be used by new drivers. Use
   342		 * &drm_gem_object_funcs.free instead.
   343		 * Compared to @gem_free_object this is not encumbered with
   344		 * &drm_device.struct_mutex legacy locking schemes.
   345		 */
   346		void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
   347	
   348		/**
   349		 * @gem_open_object:
   350		 *
   351		 * This callback is deprecated in favour of &drm_gem_object_funcs.open.
   352		 *
   353		 * Driver hook called upon gem handle creation
   354		 */
   355		int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
   356	
   357		/**
   358		 * @gem_close_object:
   359		 *
   360		 * This callback is deprecated in favour of &drm_gem_object_funcs.close.
   361		 *
   362		 * Driver hook called upon gem handle release
   363		 */
   364		void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
   365	
   366		/**
   367		 * @gem_print_info:
   368		 *
   369		 * This callback is deprecated in favour of
   370		 * &drm_gem_object_funcs.print_info.
   371		 *
   372		 * If driver subclasses struct &drm_gem_object, it can implement this
   373		 * optional hook for printing additional driver specific info.
   374		 *
   375		 * drm_printf_indent() should be used in the callback passing it the
   376		 * indent argument.
   377		 *
   378		 * This callback is called from drm_gem_print_info().
   379		 */
   380		void (*gem_print_info)(struct drm_printer *p, unsigned int indent,
   381				       const struct drm_gem_object *obj);
   382	
   383		/**
   384		 * @gem_create_object: constructor for gem objects
   385		 *
   386		 * Hook for allocating the GEM object struct, for use by the CMA and
   387		 * SHMEM GEM helpers.
   388		 */
   389		struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
   390							    size_t size);
   391		/**
   392		 * @prime_handle_to_fd:
   393		 *
   394		 * Main PRIME export function. Should be implemented with
   395		 * drm_gem_prime_handle_to_fd() for GEM based drivers.
   396		 *
   397		 * For an in-depth discussion see :ref:`PRIME buffer sharing
   398		 * documentation <prime_buffer_sharing>`.
   399		 */
   400		int (*prime_handle_to_fd)(struct drm_device *dev, struct drm_file *file_priv,
   401					uint32_t handle, uint32_t flags, int *prime_fd);
   402		/**
   403		 * @prime_fd_to_handle:
   404		 *
   405		 * Main PRIME import function. Should be implemented with
   406		 * drm_gem_prime_fd_to_handle() for GEM based drivers.
   407		 *
   408		 * For an in-depth discussion see :ref:`PRIME buffer sharing
   409		 * documentation <prime_buffer_sharing>`.
   410		 */
   411		int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file *file_priv,
   412					int prime_fd, uint32_t *handle);
   413		/**
   414		 * @gem_prime_export:
   415		 *
   416		 * Export hook for GEM drivers. Deprecated in favour of
   417		 * &drm_gem_object_funcs.export.
   418		 */
   419		struct dma_buf * (*gem_prime_export)(struct drm_gem_object *obj,
   420						     int flags);
   421		/**
   422		 * @gem_prime_import:
   423		 *
   424		 * Import hook for GEM drivers.
   425		 *
   426		 * This defaults to drm_gem_prime_import() if not set.
   427		 */
   428		struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
   429					struct dma_buf *dma_buf);
   430	
   431		/**
   432		 * @gem_prime_pin:
   433		 *
   434		 * Deprecated hook in favour of &drm_gem_object_funcs.pin.
   435		 */
   436		int (*gem_prime_pin)(struct drm_gem_object *obj);
   437	
   438		/**
   439		 * @gem_prime_unpin:
   440		 *
   441		 * Deprecated hook in favour of &drm_gem_object_funcs.unpin.
   442		 */
   443		void (*gem_prime_unpin)(struct drm_gem_object *obj);
   444	
   445	
   446		/**
   447		 * @gem_prime_get_sg_table:
   448		 *
   449		 * Deprecated hook in favour of &drm_gem_object_funcs.get_sg_table.
   450		 */
   451		struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
   452	
   453		/**
   454		 * @gem_prime_import_sg_table:
   455		 *
   456		 * Optional hook used by the PRIME helper functions
   457		 * drm_gem_prime_import() respectively drm_gem_prime_import_dev().
   458		 */
   459		struct drm_gem_object *(*gem_prime_import_sg_table)(
   460					struct drm_device *dev,
   461					struct dma_buf_attachment *attach,
   462					struct sg_table *sgt);
   463		/**
   464		 * @gem_prime_vmap:
   465		 *
   466		 * Deprecated vmap hook for GEM drivers. Please use
   467		 * &drm_gem_object_funcs.vmap instead.
   468		 */
   469		void *(*gem_prime_vmap)(struct drm_gem_object *obj);
   470	
   471		/**
   472		 * @gem_prime_vunmap:
   473		 *
   474		 * Deprecated vunmap hook for GEM drivers. Please use
   475		 * &drm_gem_object_funcs.vunmap instead.
   476		 */
   477		void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
   478	
   479		/**
   480		 * @gem_prime_mmap:
   481		 *
   482		 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
   483		 * PRIME helpers.
   484		 *
   485		 * FIXME: There's way too much duplication going on here, and also moved
   486		 * to &drm_gem_object_funcs.
   487		 */
   488		int (*gem_prime_mmap)(struct drm_gem_object *obj,
   489					struct vm_area_struct *vma);
   490	
   491		/**
   492		 * @dumb_create:
   493		 *
   494		 * This creates a new dumb buffer in the driver's backing storage manager (GEM,
   495		 * TTM or something else entirely) and returns the resulting buffer handle. This
   496		 * handle can then be wrapped up into a framebuffer modeset object.
   497		 *
   498		 * Note that userspace is not allowed to use such objects for render
   499		 * acceleration - drivers must create their own private ioctls for such a use
   500		 * case.
   501		 *
   502		 * Width, height and depth are specified in the &drm_mode_create_dumb
   503		 * argument. The callback needs to fill the handle, pitch and size for
   504		 * the created buffer.
   505		 *
   506		 * Called by the user via ioctl.
   507		 *
   508		 * Returns:
   509		 *
   510		 * Zero on success, negative errno on failure.
   511		 */
   512		int (*dumb_create)(struct drm_file *file_priv,
   513				   struct drm_device *dev,
   514				   struct drm_mode_create_dumb *args);
   515		/**
   516		 * @dumb_map_offset:
   517		 *
   518		 * Allocate an offset in the drm device node's address space to be able to
   519		 * memory map a dumb buffer.
   520		 *
   521		 * The default implementation is drm_gem_create_mmap_offset(). GEM based
   522		 * drivers must not overwrite this.
   523		 *
   524		 * Called by the user via ioctl.
   525		 *
   526		 * Returns:
   527		 *
   528		 * Zero on success, negative errno on failure.
   529		 */
   530		int (*dumb_map_offset)(struct drm_file *file_priv,
   531				       struct drm_device *dev, uint32_t handle,
   532				       uint64_t *offset);
   533		/**
   534		 * @dumb_destroy:
   535		 *
   536		 * This destroys the userspace handle for the given dumb backing storage buffer.
   537		 * Since buffer objects must be reference counted in the kernel a buffer object
   538		 * won't be immediately freed if a framebuffer modeset object still uses it.
   539		 *
   540		 * Called by the user via ioctl.
   541		 *
   542		 * The default implementation is drm_gem_dumb_destroy(). GEM based drivers
   543		 * must not overwrite this.
   544		 *
   545		 * Returns:
   546		 *
   547		 * Zero on success, negative errno on failure.
   548		 */
   549		int (*dumb_destroy)(struct drm_file *file_priv,
   550				    struct drm_device *dev,
   551				    uint32_t handle);
   552	
   553		/**
   554		 * @set_label:
   555		 *
   556		 * Label a buffer object
   557		 *
   558		 * Called by the user via ioctl.
   559		 *
   560		 * Returns:
   561		 *
   562		 * Length of label on success, negative errno on failure.
   563		 */
   564		int (*set_label)(struct drm_device *dev,
   565				 struct drm_file *file_priv,
 > 566				 struct drm_handle_label *args);
   567	
   568		/**
   569		 * @get_label:
   570		 *
   571		 * Read the label of a buffer object.
   572		 *
   573		 * Called by the user via ioctl.
   574		 *
   575		 * Returns:
   576		 *
   577		 * Length of label on success, negative errno on failiure.
   578		 */
   579		char *(*get_label)(struct drm_device *dev,
   580				   struct drm_file *file_priv,
   581				   struct drm_handle_label *args);
   582	
   583		/**
   584		 * @gem_vm_ops: Driver private ops for this object
   585		 *
   586		 * For GEM drivers this is deprecated in favour of
   587		 * &drm_gem_object_funcs.vm_ops.
   588		 */
   589		const struct vm_operations_struct *gem_vm_ops;
   590	
   591		/** @major: driver major number */
   592		int major;
   593		/** @minor: driver minor number */
   594		int minor;
   595		/** @patchlevel: driver patch level */
   596		int patchlevel;
   597		/** @name: driver name */
   598		char *name;
   599		/** @desc: driver description */
   600		char *desc;
   601		/** @date: driver date */
   602		char *date;
   603	
   604		/**
   605		 * @driver_features:
   606		 * Driver features, see &enum drm_driver_feature. Drivers can disable
   607		 * some features on a per-instance basis using
   608		 * &drm_device.driver_features.
   609		 */
   610		u32 driver_features;
   611	
   612		/**
   613		 * @ioctls:
   614		 *
   615		 * Array of driver-private IOCTL description entries. See the chapter on
   616		 * :ref:`IOCTL support in the userland interfaces
   617		 * chapter<drm_driver_ioctl>` for the full details.
   618		 */
   619	
   620		const struct drm_ioctl_desc *ioctls;
   621		/** @num_ioctls: Number of entries in @ioctls. */
   622		int num_ioctls;
   623	
   624		/**
   625		 * @fops:
   626		 *
   627		 * File operations for the DRM device node. See the discussion in
   628		 * :ref:`file operations<drm_driver_fops>` for in-depth coverage and
   629		 * some examples.
   630		 */
   631		const struct file_operations *fops;
   632	
   633		/* Everything below here is for legacy driver, never use! */
   634		/* private: */
   635	
   636		/* List of devices hanging off this driver with stealth attach. */
   637		struct list_head legacy_dev_list;
   638		int (*firstopen) (struct drm_device *);
   639		void (*preclose) (struct drm_device *, struct drm_file *file_priv);
   640		int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
   641		int (*dma_quiescent) (struct drm_device *);
   642		int (*context_dtor) (struct drm_device *dev, int context);
   643		u32 (*get_vblank_counter)(struct drm_device *dev, unsigned int pipe);
   644		int (*enable_vblank)(struct drm_device *dev, unsigned int pipe);
   645		void (*disable_vblank)(struct drm_device *dev, unsigned int pipe);
   646		int dev_priv_size;
   647	};
   648	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot May 31, 2020, 3:03 a.m. UTC | #6
Hi Rohan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-set-and-get-a-label-on-GEM-objects/20200531-000134
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: x86_64-randconfig-s021-20200531 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-243-gc100a7ab-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/drm_gem.c:974:24: sparse: sparse: incorrect type in assignment (different modifiers) @@     expected char *label @@     got char const *label @@
>> drivers/gpu/drm/drm_gem.c:974:24: sparse:     expected char *label
>> drivers/gpu/drm/drm_gem.c:974:24: sparse:     got char const *label
>> drivers/gpu/drm/drm_gem.c:992:29: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] label @@     got void * @@
>> drivers/gpu/drm/drm_gem.c:992:29: sparse:     expected unsigned long long [usertype] label
>> drivers/gpu/drm/drm_gem.c:992:29: sparse:     got void *
>> drivers/gpu/drm/drm_gem.c:1000:28: sparse: sparse: incompatible types in comparison expression (different signedness):
>> drivers/gpu/drm/drm_gem.c:1000:28: sparse:    unsigned int *
>> drivers/gpu/drm/drm_gem.c:1000:28: sparse:    int *
--
>> drivers/gpu/drm/drm_ioctl.c:636:46: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got char * @@
>> drivers/gpu/drm/drm_ioctl.c:636:46: sparse:     expected int
>> drivers/gpu/drm/drm_ioctl.c:636:46: sparse:     got char *
>> drivers/gpu/drm/drm_ioctl.c:636:24: sparse: sparse: non size-preserving pointer to integer cast

vim +974 drivers/gpu/drm/drm_gem.c

   969	
   970	void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label)
   971	{
   972		mutex_lock(&gem_obj->bo_lock);
   973		kfree(gem_obj->label);
 > 974		gem_obj->label = label;
   975		mutex_unlock(&gem_obj->bo_lock);
   976	}
   977	EXPORT_SYMBOL(drm_gem_adopt_label);
   978	
   979	int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
   980			      struct drm_handle_label *args)
   981	{
   982		struct drm_gem_object *gem_obj;
   983		int len, ret;
   984	
   985		gem_obj = drm_gem_object_lookup(file_priv, args->handle);
   986		if (!gem_obj) {
   987			DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
   988			return -ENOENT;
   989		}
   990	
   991		if (!gem_obj->label) {
 > 992			args->label = NULL;
   993			args->len = 0;
   994			return 0;
   995		}
   996	
   997		mutex_lock(&gem_obj->bo_lock);
   998		len = strlen(gem_obj->label);
   999		ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
> 1000				   min(args->len, len));
  1001		mutex_unlock(&gem_obj->bo_lock);
  1002		args->len = len;
  1003		drm_gem_object_put(gem_obj);
  1004		return ret;
  1005	}
  1006	EXPORT_SYMBOL(drm_gem_get_label);
  1007	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dan Carpenter June 2, 2020, 10:23 a.m. UTC | #7
Hi Rohan,

url:    https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-set-and-get-a-label-on-GEM-objects/20200531-000134
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: i386-randconfig-m021-20200531 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/gpu/drm/drm_gem.c:1004 drm_gem_get_label() warn: maybe return -EFAULT instead of the bytes remaining?

Old smatch warnings:
drivers/gpu/drm/drm_gem.c:910 drm_gem_open_ioctl() warn: inconsistent returns 'dev->object_name_lock'.

# https://github.com/0day-ci/linux/commit/174b10d2bdba06efe773aa0d09e682a57a00ec67
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 174b10d2bdba06efe773aa0d09e682a57a00ec67
vim +1004 drivers/gpu/drm/drm_gem.c

174b10d2bdba06 Rohan Garg 2020-05-28   979  int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
174b10d2bdba06 Rohan Garg 2020-05-28   980  		      struct drm_handle_label *args)
174b10d2bdba06 Rohan Garg 2020-05-28   981  {
174b10d2bdba06 Rohan Garg 2020-05-28   982  	struct drm_gem_object *gem_obj;
174b10d2bdba06 Rohan Garg 2020-05-28   983  	int len, ret;
174b10d2bdba06 Rohan Garg 2020-05-28   984  
174b10d2bdba06 Rohan Garg 2020-05-28   985  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
174b10d2bdba06 Rohan Garg 2020-05-28   986  	if (!gem_obj) {
174b10d2bdba06 Rohan Garg 2020-05-28   987  		DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
174b10d2bdba06 Rohan Garg 2020-05-28   988  		return -ENOENT;
174b10d2bdba06 Rohan Garg 2020-05-28   989  	}
174b10d2bdba06 Rohan Garg 2020-05-28   990  
174b10d2bdba06 Rohan Garg 2020-05-28   991  	if (!gem_obj->label) {
174b10d2bdba06 Rohan Garg 2020-05-28   992  		args->label = NULL;
174b10d2bdba06 Rohan Garg 2020-05-28   993  		args->len = 0;
174b10d2bdba06 Rohan Garg 2020-05-28   994  		return 0;
174b10d2bdba06 Rohan Garg 2020-05-28   995  	}
174b10d2bdba06 Rohan Garg 2020-05-28   996  
174b10d2bdba06 Rohan Garg 2020-05-28   997  	mutex_lock(&gem_obj->bo_lock);
174b10d2bdba06 Rohan Garg 2020-05-28   998  	len = strlen(gem_obj->label);
174b10d2bdba06 Rohan Garg 2020-05-28   999  	ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
174b10d2bdba06 Rohan Garg 2020-05-28  1000  			   min(args->len, len));

copy_to_user() returns the number of bytes remaining to be copied but
this should be:

	if (copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
			 min(args->len, len)))
		ret = -EFAULT;

Don't forget to initialize "int ret = 0;" because GCC doesn't warn about
it these days...  :/

174b10d2bdba06 Rohan Garg 2020-05-28  1001  	mutex_unlock(&gem_obj->bo_lock);
174b10d2bdba06 Rohan Garg 2020-05-28  1002  	args->len = len;
174b10d2bdba06 Rohan Garg 2020-05-28  1003  	drm_gem_object_put(gem_obj);
174b10d2bdba06 Rohan Garg 2020-05-28 @1004  	return ret;
174b10d2bdba06 Rohan Garg 2020-05-28  1005  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Rohan Garg June 9, 2020, 2:15 p.m. UTC | #8
On viernes, 29 de mayo de 2020 19:10:29 (CEST) Eric Anholt wrote:
> On Fri, May 29, 2020 at 6:44 AM Rohan Garg <rohan.garg@collabora.com> wrote:
> > Hey Eric!
> > 
> > On jueves, 28 de mayo de 2020 20:45:24 (CEST) Eric Anholt wrote:
> > > On Thu, May 28, 2020 at 10:06 AM Rohan Garg <rohan.garg@collabora.com>
> > 
> > wrote:
> > > > DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> > > > with a handle, making it easier to debug issues in userspace
> > > > applications.
> > > > 
> > > > DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> > > > with a buffer.
> > > > 
> > > > Changes in v2:
> > > >   - Hoist the IOCTL up into the drm_driver framework
> > > > 
> > > > Changes in v3:
> > > >   - Introduce a drm_gem_set_label for drivers to use internally
> > > >   
> > > >     in order to label a GEM object
> > > >   
> > > >   - Hoist string copying up into the IOCTL
> > > >   - Fix documentation
> > > >   - Move actual gem labeling into drm_gem_adopt_label
> > > > 
> > > > Changes in v4:
> > > >   - Refactor IOCTL call to only perform string duplication and move
> > > >   
> > > >     all gem lookup logic into GEM specific call
> > > > 
> > > > Changes in v5:
> > > >   - Fix issues pointed out by kbuild test robot
> > > >   - Cleanup minor issues around kfree and out/err labels
> > > >   - Fixed API documentation issues
> > > >   - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> > > >   - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> > > >   - Added some documentation for consumers of this IOCTL
> > > >   - Ensure that label's cannot be longer than PAGE_SIZE
> > > >   - Set a default label value
> > > >   - Drop useless warning
> > > >   - Properly return length of label to userspace even if
> > > >   
> > > >     userspace did not allocate memory for label.
> > > > 
> > > > Changes in v6:
> > > >   - Wrap code to make better use of 80 char limit
> > > >   - Drop redundant copies of the label
> > > >   - Protect concurrent access to labels
> > > >   - Improved documentation
> > > >   - Refactor setter/getter ioctl's to be static
> > > >   - Return EINVAL when label length > PAGE_SIZE
> > > >   - Simplify code by calling the default GEM label'ing
> > > >   
> > > >     function for all drivers using GEM
> > > >   
> > > >   - Do not error out when fetching empty labels
> > > >   - Refactor flags to the u32 type and add documentation
> > > >   - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> > > >   - Return length of copied label to userspace
> > > > 
> > > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> > > 
> > > I don't think we should land this until it actually does something
> > > with the label, that feels out of the spirit of our uapi merge rules.
> > > I would suggest looking at dma_buf_set_name(), which would produce
> > > useful output in debugfs's /dma_buf/buf_info.  But also presumably you
> > > have something in panfrost using this?
> > 
> > My current short term plan is to hook up glLabel to the labeling
> > functionality in order for userspace applications to debug exactly which
> > buffer objects could be causing faults in the kernel for speedier
> > debugging.
> 
> So, something like "when a fault happens, dump/capture names of nearby
> objects"?  That would certainly be nice to have!
> 

Pretty much, yep. This seems to be a pretty common use case at the moment.

> > The more long term plan is to label each buffer with a unique id that we
> > can correlate to the GL calls being flushed in order to be able to
> > profile (a set of)  GL calls on various platforms in order to aid driver
> > developers with performance work. This could be something that we
> > corelate on the userspace side with the help of perfetto by using this
> > [1] patch that emits ftrace events.
> 
> I'm curious how BO names are part of profiling?  Do you have the
> ability to sample instruction IP and use that to figure out where time
> is spent in shaders, or something?

The BO name itself isn't part of the profiling, but if we label our batch 
buffers, and then sample the performance counters with something like
gfx-pps [1] and co relate them then we could figure out the cost of a batch of 
GL calls that were flushed to the hardware. The plan is to use DRM scheduler 
trace markers like this one [2] to figure out when jobs get run. 

Cheers
Rohan Garg

[1] https://gitlab.freedesktop.org/Fahien/gfx-pps/
[2] https://cgit.freedesktop.org/drm/drm-misc/commit/?
id=c2c91828fbdbc5a31616f956834c85ab011392e1

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index efc0367841e2..942479dc152f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -154,8 +154,10 @@  void drm_gem_private_object_init(struct drm_device *dev,
 
 	obj->dev = dev;
 	obj->filp = NULL;
+	obj->label = NULL;
 
 	kref_init(&obj->refcount);
+	mutex_init(&obj->bo_lock);
 	obj->handle_count = 0;
 	obj->size = size;
 	dma_resv_init(&obj->_resv);
@@ -940,6 +942,69 @@  drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 	idr_destroy(&file_private->object_idr);
 }
 
+int drm_gem_set_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args)
+{
+	struct drm_gem_object *gem_obj;
+	struct pid *_pid = get_task_pid(current, PIDTYPE_PID);
+	char *label;
+
+	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
+	if (!gem_obj) {
+		DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
+		return -ENOENT;
+	}
+
+	if (args->len && args->label)
+		label = strndup_user(u64_to_user_ptr(args->label),
+				     args->len + 1);
+	else
+		label = NULL;
+
+	drm_gem_adopt_label(gem_obj, label);
+	drm_gem_object_put(gem_obj);
+	return args->len;
+}
+EXPORT_SYMBOL(drm_gem_set_label);
+
+void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label)
+{
+	mutex_lock(&gem_obj->bo_lock);
+	kfree(gem_obj->label);
+	gem_obj->label = label;
+	mutex_unlock(&gem_obj->bo_lock);
+}
+EXPORT_SYMBOL(drm_gem_adopt_label);
+
+int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args)
+{
+	struct drm_gem_object *gem_obj;
+	int len, ret;
+
+	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
+	if (!gem_obj) {
+		DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
+		return -ENOENT;
+	}
+
+	if (!gem_obj->label) {
+		args->label = NULL;
+		args->len = 0;
+		return 0;
+	}
+
+	mutex_lock(&gem_obj->bo_lock);
+	len = strlen(gem_obj->label);
+	ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
+			   min(args->len, len));
+	mutex_unlock(&gem_obj->bo_lock);
+	args->len = len;
+	drm_gem_object_put(gem_obj);
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_get_label);
+
 /**
  * drm_gem_object_release - release GEM buffer object resources
  * @obj: GEM buffer object
@@ -957,6 +1022,9 @@  drm_gem_object_release(struct drm_gem_object *obj)
 
 	dma_resv_fini(&obj->_resv);
 	drm_gem_free_mmap_offset(obj);
+
+	kfree(obj->label);
+	mutex_destroy(&obj->bo_lock);
 }
 EXPORT_SYMBOL(drm_gem_object_release);
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 2470a352730b..73fd87860100 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -161,6 +161,11 @@  int drm_gem_pin(struct drm_gem_object *obj);
 void drm_gem_unpin(struct drm_gem_object *obj);
 void *drm_gem_vmap(struct drm_gem_object *obj);
 void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
+int drm_gem_set_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args);
+void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label);
+int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args);
 
 /* drm_debugfs.c drm_debugfs_crc.c */
 #if defined(CONFIG_DEBUG_FS)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 0fb9da24eaa4..04ff75e53349 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -573,6 +573,72 @@  EXPORT_SYMBOL(drm_ioctl_permit);
 #define DRM_LEGACY_IOCTL_DEF(ioctl, _func, _flags) DRM_IOCTL_DEF(ioctl, drm_invalid_op, _flags)
 #endif
 
+/**
+ * drm_handle_set_label_ioctl - Assign a string label to a handle
+ * @dev: DRM device for the ioctl
+ * @data: user argument
+ * @file_priv: drm file-private structure
+ *
+ * This is typically targeted at user space drivers to label buffer objects
+ * with relevant information to provide human readable information about the
+ * contents of a buffer (for eg: a UBO, command buffer, shader, etc).
+ *
+ * Label length *must* not be larger than PAGE_SIZE.
+ *
+ * Returns:
+ * Length of label when setting a label succeeds, negative errno otherwise.
+ */
+
+static int drm_handle_set_label_ioctl(struct drm_device *dev,
+				      void *data, struct drm_file *file_priv)
+{
+	char *label;
+	struct drm_handle_label *args = data;
+
+	if (args->len > PAGE_SIZE)
+		return -EINVAL;
+
+	if (!dev->driver->set_label && !drm_core_check_feature(dev, DRIVER_GEM))
+		return -EOPNOTSUPP;
+
+	if (dev->driver->set_label)
+		return dev->driver->set_label(dev, file_priv, args);
+
+	if (drm_core_check_feature(dev, DRIVER_GEM))
+		return drm_gem_set_label(dev, file_priv, args);
+}
+
+/**
+ * drm_handle_get_label_ioctl - Fetches the label associated with a handle
+ * @dev: DRM device for the ioctl
+ * @data: user argument
+ * @file_priv: drm file-private structure
+ *
+ * This is targeted at user space drivers to read back the labels associated
+ * with a buffer object. You'll need to call this ioctl twice in order to be
+ * able to read back the entnire label. Optionally, userspace can also read
+ * back the label partially by passing a
+ * fixed length in `struct drm_handle_label`.
+ *
+ * Returns:
+ * Length of label copied back to userspace.
+ */
+
+static int drm_handle_get_label_ioctl(struct drm_device *dev,
+				      void *data, struct drm_file *file_priv)
+{
+	struct drm_handle_label *args = data;
+
+	if (!dev->driver->get_label && !drm_core_check_feature(dev, DRIVER_GEM))
+		return -EOPNOTSUPP;
+
+	if (dev->driver->get_label)
+		return dev->driver->get_label(dev, file_priv, args);
+
+	if (drm_core_check_feature(dev, DRIVER_GEM))
+		return drm_gem_get_label(dev, file_priv, args);
+}
+
 /* Ioctl table */
 static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_RENDER_ALLOW),
@@ -715,6 +781,10 @@  static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_HANDLE_SET_LABEL, drm_handle_set_label_ioctl,
+		      DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_HANDLE_GET_LABEL, drm_handle_get_label_ioctl,
+		      DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index bb924cddc09c..6fcb7b9ff322 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -540,6 +540,36 @@  struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/**
+	 * @set_label:
+	 *
+	 * Label a buffer object
+	 *
+	 * Called by the user via ioctl.
+	 *
+	 * Returns:
+	 *
+	 * Length of label on success, negative errno on failure.
+	 */
+	int (*set_label)(struct drm_device *dev,
+			 struct drm_file *file_priv,
+			 struct drm_handle_label *args);
+
+	/**
+	 * @get_label:
+	 *
+	 * Read the label of a buffer object.
+	 *
+	 * Called by the user via ioctl.
+	 *
+	 * Returns:
+	 *
+	 * Length of label on success, negative errno on failiure.
+	 */
+	char *(*get_label)(struct drm_device *dev,
+			   struct drm_file *file_priv,
+			   struct drm_handle_label *args);
+
 	/**
 	 * @gem_vm_ops: Driver private ops for this object
 	 *
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 2410ff0a8e86..064b080eb32f 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -250,6 +250,13 @@  struct drm_gem_object {
 	 */
 	int name;
 
+	/**
+	 * @label:
+	 *
+	 * Label for this object, should be a human readable string.
+	 */
+	char *label;
+
 	/**
 	 * @dma_buf:
 	 *
@@ -311,6 +318,13 @@  struct drm_gem_object {
 	 *
 	 */
 	const struct drm_gem_object_funcs *funcs;
+
+	/**
+	 * @bo_lock
+	 *
+	 * Protects buffer object labels
+	 */
+	struct mutex bo_lock;
 };
 
 /**
@@ -418,5 +432,10 @@  int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 int drm_gem_dumb_destroy(struct drm_file *file,
 			 struct drm_device *dev,
 			 uint32_t handle);
+int drm_gem_set_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args);
+void drm_gem_adopt_label(struct drm_gem_object *gem_obj, const char *label);
+int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
+		      struct drm_handle_label *args);
 
 #endif /* __DRM_GEM_H__ */
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 808b48a93330..2a7128271307 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -626,6 +626,27 @@  struct drm_gem_open {
 	__u64 size;
 };
 
+/** struct drm_handle_label - ioctl argument for labelling BOs.
+ *
+ * This label's a BO with a userspace label
+ *
+ */
+struct drm_handle_label {
+	/** Handle for the object being labelled. */
+	__u32 handle;
+
+	/** Label and label length (len excludes the trailing NULL).
+	 *  Label length *MUST* be smaller than PAGE_SIZE.
+	 *  Label length can be smaller than actual length when reading
+	 *  the label back.
+	 */
+	__u32 len;
+	__u64 label;
+
+	/** Flags .. Currently no flags are defined */
+	__u32 flags;
+};
+
 #define DRM_CAP_DUMB_BUFFER		0x1
 #define DRM_CAP_VBLANK_HIGH_CRTC	0x2
 #define DRM_CAP_DUMB_PREFERRED_DEPTH	0x3
@@ -947,8 +968,10 @@  extern "C" {
 #define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_array)
 #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
 #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
-
 #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
+#define DRM_IOCTL_HANDLE_SET_LABEL      DRM_IOW(0xCF, struct drm_handle_label)
+#define DRM_IOCTL_HANDLE_GET_LABEL      DRM_IOWR(0xD0, struct drm_handle_label)
+
 
 /**
  * Device specific ioctls should only be in their respective headers