[RFC 4/5] drm: Introduce drm_set_unique()

[Thierry Reding]

On Wed, Apr 23, 2014 at 10:40:57AM +0200, Daniel Vetter wrote:
> On Wed, Apr 23, 2014 at 09:17:16AM +0200, Thierry Reding wrote:
> > On Tue, Apr 22, 2014 at 05:48:07PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 22, 2014 at 05:09:32PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding 
> > > > 
> > > > Add a helper function that allows drivers to statically set the unique
> > > > name of the device. This will allow platform and USB drivers to get rid
> > > > of their DRM bus implementations and directly use drm_dev_alloc() and
> > > > drm_dev_register().
> > > > 
> > > > Signed-off-by: Thierry Reding 
> > > > ---
> > > >  drivers/gpu/drm/drm_ioctl.c | 37 +++--
> > > >  drivers/gpu/drm/drm_stub.c  |  1 +
> > > >  include/drm/drmP.h  |  3 +++
> > > >  3 files changed, 35 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > index 2dd3a6d8382b..371db3bef60c 100644
> > > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > @@ -42,6 +42,20 @@
> > > >  #include 
> > > >  #endif
> > > >  
> > > > +int drm_set_unique(struct drm_device *dev, const char *fmt, ...)
> > > 
> > > Can you please add a bit of kerneldoc for this? drm_ioctl.c isn't yet
> > > pulled into the drm reference docbook, but better to have it there
> > > already.
> > 
> > On second thought, wouldn't this be better located in drm_stub.c? It
> > isn't really related to the IOCTL code except that one of the IOCTLs now
> > uses the information set by this function. Logically I think it belongs
> > with the likes of drm_dev_alloc() and drm_dev_register().
> 
> Yeah makes sense. Tbh the entire split-up of these core drm functions is
> still a bit messy, so I don't mind if it's a bit inconsistent really. We
> can do the suffling when someone bothers with the kerneldoc for all of
> them and pulls it into the drm docbook.

I ended up doing exactly that when I wrote the drm_set_unique() docbook
pieces and I've sent out patches based on top of v2 of this patch just
now.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[RFC 4/5] drm: Introduce drm_set_unique()

[David Herrmann]

Hi

On Wed, Apr 23, 2014 at 10:40 AM, Daniel Vetter  wrote:
> On Wed, Apr 23, 2014 at 09:17:16AM +0200, Thierry Reding wrote:
>> On second thought, wouldn't this be better located in drm_stub.c? It
>> isn't really related to the IOCTL code except that one of the IOCTLs now
>> uses the information set by this function. Logically I think it belongs
>> with the likes of drm_dev_alloc() and drm_dev_register().
>
> Yeah makes sense. Tbh the entire split-up of these core drm functions is
> still a bit messy, so I don't mind if it's a bit inconsistent really. We
> can do the suffling when someone bothers with the kerneldoc for all of
> them and pulls it into the drm docbook.

During drm_dev_*() cleanup, I tried to keep the following structure:
  drm_drv.c: global drm-module setup
  drm_stub.c: drm_device allocation, registration and lifetime management
  drm_fops.c: reference-implementation of the drm file_operations

The only thing that's wrongly placed is ioctl handling (which somehow
ended up in drm_drv.c instead of drm_fops.c). drm_stub.c is where all
the generic and mandatory DRM device handling is placed so yeah, I'd
put the set_unique() there.

Thanks
David

[RFC 4/5] drm: Introduce drm_set_unique()

[Daniel Vetter]

On Wed, Apr 23, 2014 at 09:17:16AM +0200, Thierry Reding wrote:
> On Tue, Apr 22, 2014 at 05:48:07PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 22, 2014 at 05:09:32PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding 
> > > 
> > > Add a helper function that allows drivers to statically set the unique
> > > name of the device. This will allow platform and USB drivers to get rid
> > > of their DRM bus implementations and directly use drm_dev_alloc() and
> > > drm_dev_register().
> > > 
> > > Signed-off-by: Thierry Reding 
> > > ---
> > >  drivers/gpu/drm/drm_ioctl.c | 37 +++--
> > >  drivers/gpu/drm/drm_stub.c  |  1 +
> > >  include/drm/drmP.h  |  3 +++
> > >  3 files changed, 35 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 2dd3a6d8382b..371db3bef60c 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -42,6 +42,20 @@
> > >  #include 
> > >  #endif
> > >  
> > > +int drm_set_unique(struct drm_device *dev, const char *fmt, ...)
> > 
> > Can you please add a bit of kerneldoc for this? drm_ioctl.c isn't yet
> > pulled into the drm reference docbook, but better to have it there
> > already.
> 
> On second thought, wouldn't this be better located in drm_stub.c? It
> isn't really related to the IOCTL code except that one of the IOCTLs now
> uses the information set by this function. Logically I think it belongs
> with the likes of drm_dev_alloc() and drm_dev_register().

Yeah makes sense. Tbh the entire split-up of these core drm functions is
still a bit messy, so I don't mind if it's a bit inconsistent really. We
can do the suffling when someone bothers with the kerneldoc for all of
them and pulls it into the drm docbook.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC 4/5] drm: Introduce drm_set_unique()

[Thierry Reding]

On Tue, Apr 22, 2014 at 05:48:07PM +0200, Daniel Vetter wrote:
> On Tue, Apr 22, 2014 at 05:09:32PM +0200, Thierry Reding wrote:
> > From: Thierry Reding 
> > 
> > Add a helper function that allows drivers to statically set the unique
> > name of the device. This will allow platform and USB drivers to get rid
> > of their DRM bus implementations and directly use drm_dev_alloc() and
> > drm_dev_register().
> > 
> > Signed-off-by: Thierry Reding 
> > ---
> >  drivers/gpu/drm/drm_ioctl.c | 37 +++--
> >  drivers/gpu/drm/drm_stub.c  |  1 +
> >  include/drm/drmP.h  |  3 +++
> >  3 files changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 2dd3a6d8382b..371db3bef60c 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -42,6 +42,20 @@
> >  #include 
> >  #endif
> >  
> > +int drm_set_unique(struct drm_device *dev, const char *fmt, ...)
> 
> Can you please add a bit of kerneldoc for this? drm_ioctl.c isn't yet
> pulled into the drm reference docbook, but better to have it there
> already.

On second thought, wouldn't this be better located in drm_stub.c? It
isn't really related to the IOCTL code except that one of the IOCTLs now
uses the information set by this function. Logically I think it belongs
with the likes of drm_dev_alloc() and drm_dev_register().

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[RFC 4/5] drm: Introduce drm_set_unique()

[Daniel Vetter]

On Tue, Apr 22, 2014 at 05:09:32PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Add a helper function that allows drivers to statically set the unique
> name of the device. This will allow platform and USB drivers to get rid
> of their DRM bus implementations and directly use drm_dev_alloc() and
> drm_dev_register().
> 
> Signed-off-by: Thierry Reding 
> ---
>  drivers/gpu/drm/drm_ioctl.c | 37 +++--
>  drivers/gpu/drm/drm_stub.c  |  1 +
>  include/drm/drmP.h  |  3 +++
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 2dd3a6d8382b..371db3bef60c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -42,6 +42,20 @@
>  #include 
>  #endif
>  
> +int drm_set_unique(struct drm_device *dev, const char *fmt, ...)

Can you please add a bit of kerneldoc for this? drm_ioctl.c isn't yet
pulled into the drm reference docbook, but better to have it there
already. With that fixed this is

Reviewed-by: Daniel Vetter 

> +{
> + va_list ap;
> +
> + kfree(dev->unique);
> +
> + va_start(ap, fmt);
> + dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
> + va_end(ap);
> +
> + return dev->unique ? 0 : -ENOMEM;
> +}
> +EXPORT_SYMBOL(drm_set_unique);
> +
>  /**
>   * Get the bus id.
>   *
> @@ -131,13 +145,24 @@ static int drm_set_busid(struct drm_device *dev, struct 
> drm_file *file_priv)
>   if (master->unique != NULL)
>   drm_unset_busid(dev, master);
>  
> - ret = dev->driver->bus->set_busid(dev, master);
> - if (ret)
> - goto err;
> + if (dev->driver->bus && dev->driver->bus->set_busid) {
> + ret = dev->driver->bus->set_busid(dev, master);
> + if (ret) {
> + drm_unset_busid(dev, master);
> + return ret;
> + }
> + } else {
> + WARN(dev->unique == NULL,
> +  "No drm_bus.set_busid() implementation provided by %ps. "
> +  "Set the unique name explicitly using drm_set_unique().",
> +  dev->driver);
> +
> + master->unique = kstrdup(dev->unique, GFP_KERNEL);
> + if (master->unique)
> + master->unique_len = strlen(dev->unique);
> + }
> +
>   return 0;
> -err:
> - drm_unset_busid(dev, master);
> - return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 3a8e832ad151..9465cf766fe7 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -646,6 +646,7 @@ static void drm_dev_release(struct kref *ref)
>   drm_minor_free(dev, DRM_MINOR_CONTROL);
>  
>   mutex_destroy(&dev->master_mutex);
> + kfree(dev->unique);
>   kfree(dev);
>  }
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 8c80c1894b41..8fdefcdc4036 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1158,6 +1158,8 @@ struct drm_device {
>   struct drm_vma_offset_manager *vma_offset_manager;
>   /*@} */
>   int switch_power_state;
> +
> + char *unique;
>  };
>  
>  #define DRM_SWITCH_POWER_ON 0
> @@ -1238,6 +1240,7 @@ extern unsigned int drm_poll(struct file *filp, struct 
> poll_table_struct *wait);
>   /* Misc. IOCTL support (drm_ioctl.h) */
>  extern int drm_irq_by_busid(struct drm_device *dev, void *data,
>   struct drm_file *file_priv);
> +extern int drm_set_unique(struct drm_device *dev, const char *fmt, ...);
>  extern int drm_getunique(struct drm_device *dev, void *data,
>struct drm_file *file_priv);
>  extern int drm_setunique(struct drm_device *dev, void *data,
> -- 
> 1.9.2
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC 4/5] drm: Introduce drm_set_unique()

[Thierry Reding]

From: Thierry Reding 

Add a helper function that allows drivers to statically set the unique
name of the device. This will allow platform and USB drivers to get rid
of their DRM bus implementations and directly use drm_dev_alloc() and
drm_dev_register().

Signed-off-by: Thierry Reding 
---
 drivers/gpu/drm/drm_ioctl.c | 37 +++--
 drivers/gpu/drm/drm_stub.c  |  1 +
 include/drm/drmP.h  |  3 +++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2dd3a6d8382b..371db3bef60c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -42,6 +42,20 @@
 #include 
 #endif

+int drm_set_unique(struct drm_device *dev, const char *fmt, ...)
+{
+   va_list ap;
+
+   kfree(dev->unique);
+
+   va_start(ap, fmt);
+   dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
+   va_end(ap);
+
+   return dev->unique ? 0 : -ENOMEM;
+}
+EXPORT_SYMBOL(drm_set_unique);
+
 /**
  * Get the bus id.
  *
@@ -131,13 +145,24 @@ static int drm_set_busid(struct drm_device *dev, struct 
drm_file *file_priv)
if (master->unique != NULL)
drm_unset_busid(dev, master);

-   ret = dev->driver->bus->set_busid(dev, master);
-   if (ret)
-   goto err;
+   if (dev->driver->bus && dev->driver->bus->set_busid) {
+   ret = dev->driver->bus->set_busid(dev, master);
+   if (ret) {
+   drm_unset_busid(dev, master);
+   return ret;
+   }
+   } else {
+   WARN(dev->unique == NULL,
+"No drm_bus.set_busid() implementation provided by %ps. "
+"Set the unique name explicitly using drm_set_unique().",
+dev->driver);
+
+   master->unique = kstrdup(dev->unique, GFP_KERNEL);
+   if (master->unique)
+   master->unique_len = strlen(dev->unique);
+   }
+
return 0;
-err:
-   drm_unset_busid(dev, master);
-   return ret;
 }

 /**
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 3a8e832ad151..9465cf766fe7 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -646,6 +646,7 @@ static void drm_dev_release(struct kref *ref)
drm_minor_free(dev, DRM_MINOR_CONTROL);

mutex_destroy(&dev->master_mutex);
+   kfree(dev->unique);
kfree(dev);
 }

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 8c80c1894b41..8fdefcdc4036 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1158,6 +1158,8 @@ struct drm_device {
struct drm_vma_offset_manager *vma_offset_manager;
/*@} */
int switch_power_state;
+
+   char *unique;
 };

 #define DRM_SWITCH_POWER_ON 0
@@ -1238,6 +1240,7 @@ extern unsigned int drm_poll(struct file *filp, struct 
poll_table_struct *wait);
/* Misc. IOCTL support (drm_ioctl.h) */
 extern int drm_irq_by_busid(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+extern int drm_set_unique(struct drm_device *dev, const char *fmt, ...);
 extern int drm_getunique(struct drm_device *dev, void *data,
 struct drm_file *file_priv);
 extern int drm_setunique(struct drm_device *dev, void *data,
-- 
1.9.2