Re: [PATCH 1/2] Add new drmOpenWithType function (v3)

[Frank Binns <frank.binns@imgtec.com>]

On 02/02/15 10:06, Jammy Zhou wrote:
> v2: Add drmGetMinorBase, and call drmOpenWithType in drmOpen
> v3: Pass 'type' to drmOpenByBusid and drmOpenDevice in drmOpenByName
>
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>  xf86drm.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
>  xf86drm.h |  9 ++++++++-
>  2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 345325a..810edfa 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -85,10 +85,6 @@
>  
>  #define DRM_MSG_VERBOSITY 3
>  
> -#define DRM_NODE_CONTROL 0
> -#define DRM_NODE_PRIMARY 1
> -#define DRM_NODE_RENDER 2
> -
>  static drmServerInfoPtr drm_server_info;
>  
>  void drmSetServerInfo(drmServerInfoPtr info)
> @@ -493,11 +489,24 @@ int drmAvailable(void)
>      return retval;
>  }
>  
> +static int drmGetMinorBase(int type)
> +{
> +    switch (type) {
> +    case DRM_NODE_PRIMARY:
> +    default:
> +        return 0;
> +    case DRM_NODE_CONTROL:
> +        return 64;
> +    case DRM_NODE_RENDER:
> +        return 128;
> +    };
Wouldn't it be more sensible to return -1 in the default case given that
there's no validation of the 'type' argument in drmOpenWithType? It
feels wrong defaulting to the primary node in this case.

> +}
>  
>  /**
>   * Open the device by bus ID.
>   *
>   * \param busid bus ID.
> + * \param type device node type.
>   *
>   * \return a file descriptor on success, or a negative value on error.
>   *
> @@ -507,16 +516,17 @@ int drmAvailable(void)
>   *
>   * \sa drmOpenMinor() and drmGetBusid().
>   */
> -static int drmOpenByBusid(const char *busid)
> +static int drmOpenByBusid(const char *busid, int type)
>  {
>      int        i, pci_domain_ok = 1;
>      int        fd;
>      const char *buf;
>      drmSetVersion sv;
> +    int        base = drmGetMinorBase(type);
>  
>      drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
> -    for (i = 0; i < DRM_MAX_MINOR; i++) {
> -	fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
> +    for (i = base; i < base + DRM_MAX_MINOR; i++) {
> +	fd = drmOpenMinor(i, 1, type);
>  	drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
>  	if (fd >= 0) {
>  	    /* We need to try for 1.4 first for proper PCI domain support
> @@ -556,6 +566,7 @@ static int drmOpenByBusid(const char *busid)
>   * Open the device by name.
>   *
>   * \param name driver name.
> + * \param type the device node type.
>   * 
>   * \return a file descriptor on success, or a negative value on error.
>   * 
> @@ -566,19 +577,20 @@ static int drmOpenByBusid(const char *busid)
>   * 
>   * \sa drmOpenMinor(), drmGetVersion() and drmGetBusid().
>   */
> -static int drmOpenByName(const char *name)
> +static int drmOpenByName(const char *name, int type)
>  {
>      int           i;
>      int           fd;
>      drmVersionPtr version;
>      char *        id;
> +    int           base = drmGetMinorBase(type);
>  
>      /*
>       * Open the first minor number that matches the driver name and isn't
>       * already in use.  If it's in use it will have a busid assigned already.
>       */
> -    for (i = 0; i < DRM_MAX_MINOR; i++) {
> -	if ((fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY)) >= 0) {
> +    for (i = base; i < base + DRM_MAX_MINOR; i++) {
> +	if ((fd = drmOpenMinor(i, 1, type)) >= 0) {
>  	    if ((version = drmGetVersion(fd))) {
>  		if (!strcmp(version->name, name)) {
>  		    drmFreeVersion(version);
> @@ -620,9 +632,9 @@ static int drmOpenByName(const char *name)
>  			for (devstring = ++pt; *pt && *pt != ' '; ++pt)
>  			    ;
>  			if (*pt) { /* Found busid */
> -			    return drmOpenByBusid(++pt);
> +			    return drmOpenByBusid(++pt, type);
>  			} else { /* No busid */
> -			    return drmOpenDevice(strtol(devstring, NULL, 0),i, DRM_NODE_PRIMARY);
> +			    return drmOpenDevice(strtol(devstring, NULL, 0),i, type);
>  			}
>  		    }
>  		}
> @@ -652,8 +664,29 @@ static int drmOpenByName(const char *name)
>   */
>  int drmOpen(const char *name, const char *busid)
>  {
> +    return drmOpenWithType(name, busid, DRM_NODE_PRIMARY);
> +}
> +
> +/**
> + * Open the DRM device with specified type.
> + *
> + * Looks up the specified name and bus ID, and opens the device found.  The
> + * entry in /dev/dri is created if necessary and if called by root.
> + *
> + * \param name driver name. Not referenced if bus ID is supplied.
> + * \param busid bus ID. Zero if not known.
> + * \param type the device node type to open, PRIMARY, CONTROL or RENDER
> + *
> + * \return a file descriptor on success, or a negative value on error.
> + *
> + * \internal
> + * It calls drmOpenByBusid() if \p busid is specified or drmOpenByName()
> + * otherwise.
> + */
> +int drmOpenWithType(const char *name, const char *busid, int type)
> +{
>      if (!drmAvailable() && name != NULL && drm_server_info) {
> -	/* try to load the kernel */
> +	/* try to load the kernel module */
>  	if (!drm_server_info->load_module(name)) {
>  	    drmMsg("[drm] failed to load kernel module \"%s\"\n", name);
>  	    return -1;
> @@ -661,13 +694,13 @@ int drmOpen(const char *name, const char *busid)
>      }
>  
>      if (busid) {
> -	int fd = drmOpenByBusid(busid);
> +	int fd = drmOpenByBusid(busid, type);
>  	if (fd >= 0)
>  	    return fd;
>      }
>      
>      if (name)
> -	return drmOpenByName(name);
> +	return drmOpenByName(name, type);
>  
>      return -1;
>  }
> diff --git a/xf86drm.h b/xf86drm.h
> index bfd0670..f145d42 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -552,7 +552,14 @@ do {	register unsigned int __old __asm("o0");		\
>  /* General user-level programmer's API: unprivileged */
>  extern int           drmAvailable(void);
>  extern int           drmOpen(const char *name, const char *busid);
> -extern int drmOpenControl(int minor);
> +
> +#define DRM_NODE_CONTROL 0
> +#define DRM_NODE_PRIMARY 1
> +#define DRM_NODE_RENDER  2
Nit, now that these are public it might be nice to have primary first,
.i.e.:

#define DRM_NODE_PRIMARY 0
#define DRM_NODE_CONTROL 1
#define DRM_NODE_RENDER 2

> +extern int           drmOpenWithType(const char *name, const char *busid,
> +                                     int type);
> +
> +extern int           drmOpenControl(int minor);
>  extern int           drmOpenRender(int minor);
>  extern int           drmClose(int fd);
>  extern drmVersionPtr drmGetVersion(int fd);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel