| Message ID | 20200528170604.22476-1-rohan.garg@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects | expand |
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.
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
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?
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
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
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
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
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
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
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(-)