Re: [PATCH 16/16] drm: document drm_auth.c

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 18, 2016 at 12:46:38AM +0100, Emil Velikov wrote:
> On 17 June 2016 at 08:33, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > @@ -64,16 +73,6 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
> >         return ret < 0 ? ret : 0;
> >  }
> >
> > -/**
> > - * drm_authmagic - Authenticate client with a magic
> > - * @dev: DRM device to operate on
> > - * @data: ioctl data containing the drm_auth object
> > - * @file_priv: DRM file that performs the operation
> > - *
> > - * This looks up a DRM client by the passed magic and authenticates it.
> > - *
> > - * Returns: 0 on success, negative error code on failure.
> > - */
> Why is this and drm_getmagic()'s documetation going away ? Kernel doc
> isn't restricted to EXPORTED_SYMBOL(s) only, is it ?

No, but imo the documentation for the drm core&helpers should be aimed at
driver writers. And driver writers can only use EXPORT_SYMBOL stuff (or
from headers), which is why I think it's good to kick out everything else
to avoid too much clutter. It's already a daunting thing to get started
with a new drm driver.

Of course we can keep the comments as normal comments, but for these here
I didn't see the value.

Also note that this is just for the drm core/helpers. In the i915 driver
documentation we instead try to document non-static stuff (since that's
exposed to other parts), but just as a rough guideline. Since often our
source file split doesn't make that much sense, or is too monolithic.

In both cases the goal is to give a useful guideline to users of a piece
of code (calling it or otherwise using), not developers changing the
implementation details themselves.
-Daniel

> 
> >  int drm_authmagic(struct drm_device *dev, void *data,
> >                   struct drm_file *file_priv)
> >  {
> > @@ -126,16 +125,6 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
> >         return ret;
> >  }
> >
> > -/*
> > - * drm_new_set_master - Allocate a new master object and become master for the
> > - * associated master realm.
> > - *
> > - * @dev: The associated device.
> > - * @fpriv: File private identifying the client.
> > - *
> > - * This function must be called with dev::master_mutex held.
> > - * Returns negative error code on failure. Zero on success.
> > - */
> >  static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> >  {
> >         struct drm_master *old_master;
> > @@ -288,12 +277,28 @@ out:
> >         mutex_unlock(&dev->master_mutex);
> >  }
> >
> > +/**
> > + * drm_is_current_master - checks whether this master is the current one
> s/this master is the current one/@fpriv is the current master/ perhaps ?
> 
> > + * @fpriv: DRM file private
> > + *
> > + * Checks whether @fpriv is a master and that it is the current master on its
> s/a master and that it is the current/the current/
> 
> > + * device. This decides whether a client is allowed to run DRM_MASTER IOCTLs.
> > + *
> > + * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
> > + * - the current master is assumed to own the non-shareable display hardware.
> > + */
> >  bool drm_is_current_master(struct drm_file *fpriv)
> >  {
> >         return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
> >  }
> >  EXPORT_SYMBOL(drm_is_current_master);
> >
> > +/**
> > + * drm_master_get - reference a master pointer
> > + * @master: struct &drm_master
> > + *
> > + * Increments the reference count of @master.
> Bikeshed: s/@master./@master and returns a pointer to @master./
> 
> > + */
> >  struct drm_master *drm_master_get(struct drm_master *master)
> >  {
> >         kref_get(&master->refcount);
> > @@ -316,6 +321,12 @@ static void drm_master_destroy(struct kref *kref)
> >         kfree(master);
> >  }
> >
> > +/**
> > + * drm_master_put - unreference and clear a master pointer
> > + * @master: pointer to a pointer of struct &drm_master
> > + *
> > + * This decrements the &drm_master behind @master and sets it to NULL.
> > + */
> >  void drm_master_put(struct drm_master **master)
> >  {
> >         kref_put(&(*master)->refcount, drm_master_destroy);
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 81083f98d155..871af372662d 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -39,6 +39,7 @@
> >  #include <drm/drm_fourcc.h>
> >  #include <drm/drm_modeset_lock.h>
> >  #include <drm/drm_atomic.h>
> > +#include <drm/drm_auth.h>
> >
> >  #include "drm_crtc_internal.h"
> >  #include "drm_internal.h"
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index a0c1d172954d..88796a383e40 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -30,6 +30,7 @@
> >
> >  #include <drm/drmP.h>
> >  #include <drm/drm_core.h>
> > +#include <drm/drm_auth.h>
> >  #include "drm_legacy.h"
> >  #include "drm_internal.h"
> >  #include "drm_crtc_internal.h"
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9fa9698fe247..0f8632c93e95 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -47,6 +47,7 @@
> >  #include <drm/intel-gtt.h>
> >  #include <drm/drm_legacy.h> /* for struct drm_dma_handle */
> >  #include <drm/drm_gem.h>
> > +#include <drm/drm_auth.h>
> >
> >  #include "i915_params.h"
> >  #include "i915_reg.h"
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > index 1980e2a28265..9a90f824814e 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > @@ -32,6 +32,7 @@
> >  #include <drm/drmP.h>
> >  #include <drm/vmwgfx_drm.h>
> >  #include <drm/drm_hashtab.h>
> > +#include <drm/drm_auth.h>
> >  #include <linux/suspend.h>
> >  #include <drm/ttm/ttm_bo_driver.h>
> >  #include <drm/ttm/ttm_object.h>
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 761b20332321..d22ba6bf2299 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -86,6 +86,7 @@ struct drm_local_map;
> >  struct drm_device_dma;
> >  struct drm_dma_handle;
> >  struct drm_gem_object;
> > +struct drm_master;
> >
> >  struct device_node;
> >  struct videomode;
> > @@ -373,30 +374,6 @@ struct drm_lock_data {
> >         int idle_has_lock;
> >  };
> >
> > -/**
> > - * struct drm_master - drm master structure
> > - *
> > - * @refcount: Refcount for this master object.
> > - * @dev: Link back to the DRM device
> > - * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
> > - * @unique_len: Length of unique field. Protected by drm_global_mutex.
> > - * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
> > - * @lock: DRI lock information.
> > - * @driver_priv: Pointer to driver-private information.
> > - *
> > - * Note that master structures are only relevant for the legacy/primary device
> > - * nodes, hence there can only be one per device, not one per drm_minor.
> > - */
> > -struct drm_master {
> > -       struct kref refcount;
> > -       struct drm_device *dev;
> > -       char *unique;
> > -       int unique_len;
> > -       struct idr magic_map;
> > -       struct drm_lock_data lock;
> > -       void *driver_priv;
> > -};
> > -
> >  /* Flags and return codes for get_vblank_timestamp() driver function. */
> >  #define DRM_CALLED_FROM_VBLIRQ 1
> >  #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0)
> > @@ -1008,11 +985,6 @@ static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc
> >         return &crtc->dev->vblank[drm_crtc_index(crtc)].queue;
> >  }
> >
> > -/* drm_auth.c */
> > -struct drm_master *drm_master_get(struct drm_master *master);
> > -void drm_master_put(struct drm_master **master);
> > -bool drm_is_current_master(struct drm_file *fpriv);
> > -
> >  /* drm_drv.c */
> >  void drm_put_dev(struct drm_device *dev);
> >  void drm_unplug_dev(struct drm_device *dev);
> > diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> > new file mode 100644
> > index 000000000000..610223b0481b
> > --- /dev/null
> > +++ b/include/drm/drm_auth.h
> > @@ -0,0 +1,59 @@
> > +/*
> > + * Internal Header for the Direct Rendering Manager
> > + *
> > + * Copyright 2016 Intel Corporation
> > + *
> > + * Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef _DRM_AUTH_H_
> > +#define _DRM_AUTH_H_
> > +
> > +/**
> > + * struct drm_master - drm master structure
> > + *
> > + * @refcount: Refcount for this master object.
> > + * @dev: Link back to the DRM device
> > + * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
> > + * @unique_len: Length of unique field. Protected by drm_global_mutex.
> > + * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
> > + * @lock: DRI lock information.
> > + * @driver_priv: Pointer to driver-private information.
> > + *
> > + * Note that master structures are only relevant for the legacy/primary device
> > + * nodes, hence there can only be one per device, not one per drm_minor.
> > + */
> > +struct drm_master {
> > +       struct kref refcount;
> > +       struct drm_device *dev;
> > +       char *unique;
> > +       int unique_len;
> > +       struct idr magic_map;
> > +       struct drm_lock_data lock;
> > +       void *driver_priv;
> > +};
> > +
> > +struct drm_master *drm_master_get(struct drm_master *master);
> > +void drm_master_put(struct drm_master **master);
> > +bool drm_is_current_master(struct drm_file *fpriv);
> > +
> > +#endif
> > diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
> > index a5ef2c7e40f8..cf0e7d89bcdf 100644
> > --- a/include/drm/drm_legacy.h
> > +++ b/include/drm/drm_legacy.h
> > @@ -1,6 +1,8 @@
> >  #ifndef __DRM_DRM_LEGACY_H__
> >  #define __DRM_DRM_LEGACY_H__
> >
> > +#include <drm/drm_auth.h>
> > +
> >  /*
> >   * Legacy driver interfaces for the Direct Rendering Manager
> >   *
> > --
> > 2.8.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx