drm: add drm device name

Submitted by Jiang, Sonny on Sept. 3, 2019, 9:41 p.m.

Details

Message ID 20190903214040.2386-1-sonny.jiang@amd.com
State New
Headers show
Series "drm: add drm device name" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Jiang, Sonny Sept. 3, 2019, 9:41 p.m.
Add DRM device name and use DRM_IOCTL_VERSION ioctl drmVersion::desc passing it to user space
instead of unused DRM driver name descriptor.

Change-Id: I809f6d3e057111417efbe8fa7cab8f0113ba4b21
Signed-off-by: Sonny Jiang <sonny.jiang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 ++
 drivers/gpu/drm/drm_drv.c                  | 17 +++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c                |  2 +-
 include/drm/drm_device.h                   |  3 +++
 include/drm/drm_drv.h                      |  1 +
 5 files changed, 24 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 67b09cb2a9e2..8f0971cea363 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2809,6 +2809,8 @@  int amdgpu_device_init(struct amdgpu_device *adev,
 	/* init the mode config */
 	drm_mode_config_init(adev->ddev);
 
+	drm_dev_set_name(adev->ddev, amdgpu_asic_name[adev->asic_type]);
+
 	r = amdgpu_device_ip_init(adev);
 	if (r) {
 		/* failed in exclusive mode due to timeout */
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 862621494a93..6c33879bb538 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -802,6 +802,7 @@  void drm_dev_fini(struct drm_device *dev)
 	mutex_destroy(&dev->struct_mutex);
 	drm_legacy_destroy_members(dev);
 	kfree(dev->unique);
+	kfree(dev->name);
 }
 EXPORT_SYMBOL(drm_dev_fini);
 
@@ -1078,6 +1079,22 @@  int drm_dev_set_unique(struct drm_device *dev, const char *name)
 }
 EXPORT_SYMBOL(drm_dev_set_unique);
 
+/**
+ * drm_dev_set_name - Set the name of a DRM device
+ * @dev: device of which to set the name
+ * @name: name to be set
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_dev_set_name(struct drm_device *dev, const char *name)
+{
+	kfree(dev->name);
+	dev->name = kstrdup(name, GFP_KERNEL);
+
+	return dev->name ? 0 : -ENOMEM;
+}
+EXPORT_SYMBOL(drm_dev_set_name);
+
 /*
  * DRM Core
  * The DRM core module initializes all global DRM objects and makes them
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2263e3ddd822..61f02965106b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -506,7 +506,7 @@  int drm_version(struct drm_device *dev, void *data,
 				dev->driver->date);
 	if (!err)
 		err = drm_copy_field(version->desc, &version->desc_len,
-				dev->driver->desc);
+				dev->name);
 
 	return err;
 }
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 7f9ef709b2b6..e29912c484e4 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -123,6 +123,9 @@  struct drm_device {
 	/** @unique: Unique name of the device */
 	char *unique;
 
+	/** @name: device name */
+	char *name;
+
 	/**
 	 * @struct_mutex:
 	 *
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 68ca736c548d..f742e2bde467 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -798,6 +798,7 @@  static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
 
 
 int drm_dev_set_unique(struct drm_device *dev, const char *name);
+int drm_dev_set_name(struct drm_device *dev, const char *name);
 
 
 #endif

Comments


On Fri, Sep 6, 2019 at 3:16 PM Marek Olšák <maraeo@gmail.com> wrote:
>
> + dri-devel
>
> On Tue, Sep 3, 2019 at 5:41 PM Jiang, Sonny <Sonny.Jiang@amd.com> wrote:
>>
>> Add DRM device name and use DRM_IOCTL_VERSION ioctl drmVersion::desc passing it to user space
>> instead of unused DRM driver name descriptor.
>>
>> Change-Id: I809f6d3e057111417efbe8fa7cab8f0113ba4b21
>> Signed-off-by: Sonny Jiang <sonny.jiang@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 ++
>>  drivers/gpu/drm/drm_drv.c                  | 17 +++++++++++++++++
>>  drivers/gpu/drm/drm_ioctl.c                |  2 +-
>>  include/drm/drm_device.h                   |  3 +++
>>  include/drm/drm_drv.h                      |  1 +
>>  5 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 67b09cb2a9e2..8f0971cea363 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2809,6 +2809,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>         /* init the mode config */
>>         drm_mode_config_init(adev->ddev);
>>
>> +       drm_dev_set_name(adev->ddev, amdgpu_asic_name[adev->asic_type]);
>> +
>>         r = amdgpu_device_ip_init(adev);
>>         if (r) {
>>                 /* failed in exclusive mode due to timeout */
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 862621494a93..6c33879bb538 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -802,6 +802,7 @@ void drm_dev_fini(struct drm_device *dev)
>>         mutex_destroy(&dev->struct_mutex);
>>         drm_legacy_destroy_members(dev);
>>         kfree(dev->unique);
>> +       kfree(dev->name);
>>  }
>>  EXPORT_SYMBOL(drm_dev_fini);
>>
>> @@ -1078,6 +1079,22 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>>  }
>>  EXPORT_SYMBOL(drm_dev_set_unique);
>>
>> +/**
>> + * drm_dev_set_name - Set the name of a DRM device
>> + * @dev: device of which to set the name
>> + * @name: name to be set
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int drm_dev_set_name(struct drm_device *dev, const char *name)
>> +{
>> +       kfree(dev->name);
>> +       dev->name = kstrdup(name, GFP_KERNEL);
>> +
>> +       return dev->name ? 0 : -ENOMEM;
>> +}
>> +EXPORT_SYMBOL(drm_dev_set_name);
>> +
>>  /*
>>   * DRM Core
>>   * The DRM core module initializes all global DRM objects and makes them
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 2263e3ddd822..61f02965106b 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -506,7 +506,7 @@ int drm_version(struct drm_device *dev, void *data,
>>                                 dev->driver->date);
>>         if (!err)
>>                 err = drm_copy_field(version->desc, &version->desc_len,
>> -                               dev->driver->desc);
>> +                               dev->name);

I suspect this needs to be something like dev->name ? dev->name :
dev->driver->desc

Or somewhere something needs to arrange for dev->name to default to
dev->driver->desc

And maybe this should be dev->desc instead of dev->name.. that at
least seems less confusing to me.

other than that, I don't see a big problem

BR,
-R

>>
>>         return err;
>>  }
>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>> index 7f9ef709b2b6..e29912c484e4 100644
>> --- a/include/drm/drm_device.h
>> +++ b/include/drm/drm_device.h
>> @@ -123,6 +123,9 @@ struct drm_device {
>>         /** @unique: Unique name of the device */
>>         char *unique;
>>
>> +       /** @name: device name */
>> +       char *name;
>> +
>>         /**
>>          * @struct_mutex:
>>          *
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 68ca736c548d..f742e2bde467 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -798,6 +798,7 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
>>
>>
>>  int drm_dev_set_unique(struct drm_device *dev, const char *name);
>> +int drm_dev_set_name(struct drm_device *dev, const char *name);
>>
>>
>>  #endif
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Sep 7, 2019 at 3:18 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Sep 6, 2019 at 3:16 PM Marek Olšák <maraeo@gmail.com> wrote:
> >
> > + dri-devel
> >
> > On Tue, Sep 3, 2019 at 5:41 PM Jiang, Sonny <Sonny.Jiang@amd.com> wrote:
> >>
> >> Add DRM device name and use DRM_IOCTL_VERSION ioctl drmVersion::desc passing it to user space
> >> instead of unused DRM driver name descriptor.
> >>
> >> Change-Id: I809f6d3e057111417efbe8fa7cab8f0113ba4b21
> >> Signed-off-by: Sonny Jiang <sonny.jiang@amd.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 ++
> >>  drivers/gpu/drm/drm_drv.c                  | 17 +++++++++++++++++
> >>  drivers/gpu/drm/drm_ioctl.c                |  2 +-
> >>  include/drm/drm_device.h                   |  3 +++
> >>  include/drm/drm_drv.h                      |  1 +
> >>  5 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 67b09cb2a9e2..8f0971cea363 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -2809,6 +2809,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >>         /* init the mode config */
> >>         drm_mode_config_init(adev->ddev);
> >>
> >> +       drm_dev_set_name(adev->ddev, amdgpu_asic_name[adev->asic_type]);
> >> +
> >>         r = amdgpu_device_ip_init(adev);
> >>         if (r) {
> >>                 /* failed in exclusive mode due to timeout */
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 862621494a93..6c33879bb538 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -802,6 +802,7 @@ void drm_dev_fini(struct drm_device *dev)
> >>         mutex_destroy(&dev->struct_mutex);
> >>         drm_legacy_destroy_members(dev);
> >>         kfree(dev->unique);
> >> +       kfree(dev->name);
> >>  }
> >>  EXPORT_SYMBOL(drm_dev_fini);
> >>
> >> @@ -1078,6 +1079,22 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
> >>  }
> >>  EXPORT_SYMBOL(drm_dev_set_unique);
> >>
> >> +/**
> >> + * drm_dev_set_name - Set the name of a DRM device
> >> + * @dev: device of which to set the name
> >> + * @name: name to be set
> >> + *
> >> + * Return: 0 on success or a negative error code on failure.
> >> + */
> >> +int drm_dev_set_name(struct drm_device *dev, const char *name)
> >> +{
> >> +       kfree(dev->name);
> >> +       dev->name = kstrdup(name, GFP_KERNEL);
> >> +
> >> +       return dev->name ? 0 : -ENOMEM;
> >> +}
> >> +EXPORT_SYMBOL(drm_dev_set_name);
> >> +
> >>  /*
> >>   * DRM Core
> >>   * The DRM core module initializes all global DRM objects and makes them
> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >> index 2263e3ddd822..61f02965106b 100644
> >> --- a/drivers/gpu/drm/drm_ioctl.c
> >> +++ b/drivers/gpu/drm/drm_ioctl.c
> >> @@ -506,7 +506,7 @@ int drm_version(struct drm_device *dev, void *data,
> >>                                 dev->driver->date);
> >>         if (!err)
> >>                 err = drm_copy_field(version->desc, &version->desc_len,
> >> -                               dev->driver->desc);
> >> +                               dev->name);
>
> I suspect this needs to be something like dev->name ? dev->name :
> dev->driver->desc
>
> Or somewhere something needs to arrange for dev->name to default to
> dev->driver->desc
>
> And maybe this should be dev->desc instead of dev->name.. that at
> least seems less confusing to me.
>
> other than that, I don't see a big problem

(recap from irc)

I thought we're using this as essentially an uapi identifier, so that
you know which kind of ioctl set a driver supports. Not so big deal on
pci, where we match against pci ids anyway, kinda bigger deal where
that's not around. Listing codenames and or something else that
changes all the time feels a bit silly for that. Imo if you just want
to expose this to userspace, stuff it into an amdgpu info/query ioctl.

So what do you need this for exactly, where's the userspace that needs this?
-Daniel

>
> BR,
> -R
>
> >>
> >>         return err;
> >>  }
> >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> >> index 7f9ef709b2b6..e29912c484e4 100644
> >> --- a/include/drm/drm_device.h
> >> +++ b/include/drm/drm_device.h
> >> @@ -123,6 +123,9 @@ struct drm_device {
> >>         /** @unique: Unique name of the device */
> >>         char *unique;
> >>
> >> +       /** @name: device name */
> >> +       char *name;
> >> +
> >>         /**
> >>          * @struct_mutex:
> >>          *
> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> index 68ca736c548d..f742e2bde467 100644
> >> --- a/include/drm/drm_drv.h
> >> +++ b/include/drm/drm_drv.h
> >> @@ -798,6 +798,7 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
> >>
> >>
> >>  int drm_dev_set_unique(struct drm_device *dev, const char *name);
> >> +int drm_dev_set_name(struct drm_device *dev, const char *name);
> >>
> >>
> >>  #endif
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, 07 Sep 2019, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Sep 7, 2019 at 3:18 AM Rob Clark <robdclark@gmail.com> wrote:
>>
>> On Fri, Sep 6, 2019 at 3:16 PM Marek Olšák <maraeo@gmail.com> wrote:
>> >
>> > + dri-devel
>> >
>> > On Tue, Sep 3, 2019 at 5:41 PM Jiang, Sonny <Sonny.Jiang@amd.com> wrote:
>> >>
>> >> Add DRM device name and use DRM_IOCTL_VERSION ioctl drmVersion::desc passing it to user space
>> >> instead of unused DRM driver name descriptor.
>> >>
>> >> Change-Id: I809f6d3e057111417efbe8fa7cab8f0113ba4b21
>> >> Signed-off-by: Sonny Jiang <sonny.jiang@amd.com>
>> >> ---
>> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 ++
>> >>  drivers/gpu/drm/drm_drv.c                  | 17 +++++++++++++++++
>> >>  drivers/gpu/drm/drm_ioctl.c                |  2 +-
>> >>  include/drm/drm_device.h                   |  3 +++
>> >>  include/drm/drm_drv.h                      |  1 +
>> >>  5 files changed, 24 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >> index 67b09cb2a9e2..8f0971cea363 100644
>> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >> @@ -2809,6 +2809,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>> >>         /* init the mode config */
>> >>         drm_mode_config_init(adev->ddev);
>> >>
>> >> +       drm_dev_set_name(adev->ddev, amdgpu_asic_name[adev->asic_type]);
>> >> +
>> >>         r = amdgpu_device_ip_init(adev);
>> >>         if (r) {
>> >>                 /* failed in exclusive mode due to timeout */
>> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> >> index 862621494a93..6c33879bb538 100644
>> >> --- a/drivers/gpu/drm/drm_drv.c
>> >> +++ b/drivers/gpu/drm/drm_drv.c
>> >> @@ -802,6 +802,7 @@ void drm_dev_fini(struct drm_device *dev)
>> >>         mutex_destroy(&dev->struct_mutex);
>> >>         drm_legacy_destroy_members(dev);
>> >>         kfree(dev->unique);
>> >> +       kfree(dev->name);
>> >>  }
>> >>  EXPORT_SYMBOL(drm_dev_fini);
>> >>
>> >> @@ -1078,6 +1079,22 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>> >>  }
>> >>  EXPORT_SYMBOL(drm_dev_set_unique);
>> >>
>> >> +/**
>> >> + * drm_dev_set_name - Set the name of a DRM device
>> >> + * @dev: device of which to set the name
>> >> + * @name: name to be set
>> >> + *
>> >> + * Return: 0 on success or a negative error code on failure.
>> >> + */
>> >> +int drm_dev_set_name(struct drm_device *dev, const char *name)
>> >> +{
>> >> +       kfree(dev->name);
>> >> +       dev->name = kstrdup(name, GFP_KERNEL);
>> >> +
>> >> +       return dev->name ? 0 : -ENOMEM;
>> >> +}
>> >> +EXPORT_SYMBOL(drm_dev_set_name);
>> >> +
>> >>  /*
>> >>   * DRM Core
>> >>   * The DRM core module initializes all global DRM objects and makes them
>> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> >> index 2263e3ddd822..61f02965106b 100644
>> >> --- a/drivers/gpu/drm/drm_ioctl.c
>> >> +++ b/drivers/gpu/drm/drm_ioctl.c
>> >> @@ -506,7 +506,7 @@ int drm_version(struct drm_device *dev, void *data,
>> >>                                 dev->driver->date);
>> >>         if (!err)
>> >>                 err = drm_copy_field(version->desc, &version->desc_len,
>> >> -                               dev->driver->desc);
>> >> +                               dev->name);
>>
>> I suspect this needs to be something like dev->name ? dev->name :
>> dev->driver->desc
>>
>> Or somewhere something needs to arrange for dev->name to default to
>> dev->driver->desc
>>
>> And maybe this should be dev->desc instead of dev->name.. that at
>> least seems less confusing to me.
>>
>> other than that, I don't see a big problem
>
> (recap from irc)
>
> I thought we're using this as essentially an uapi identifier, so that
> you know which kind of ioctl set a driver supports. Not so big deal on
> pci, where we match against pci ids anyway, kinda bigger deal where
> that's not around. Listing codenames and or something else that
> changes all the time feels a bit silly for that. Imo if you just want
> to expose this to userspace, stuff it into an amdgpu info/query ioctl.
>
> So what do you need this for exactly, where's the userspace that needs
> this?

Indeed; using this e.g. for changing userspace behaviour would seem
wrong.

BR,
Jani.


> -Daniel
>
>>
>> BR,
>> -R
>>
>> >>
>> >>         return err;
>> >>  }
>> >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>> >> index 7f9ef709b2b6..e29912c484e4 100644
>> >> --- a/include/drm/drm_device.h
>> >> +++ b/include/drm/drm_device.h
>> >> @@ -123,6 +123,9 @@ struct drm_device {
>> >>         /** @unique: Unique name of the device */
>> >>         char *unique;
>> >>
>> >> +       /** @name: device name */
>> >> +       char *name;
>> >> +
>> >>         /**
>> >>          * @struct_mutex:
>> >>          *
>> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> >> index 68ca736c548d..f742e2bde467 100644
>> >> --- a/include/drm/drm_drv.h
>> >> +++ b/include/drm/drm_drv.h
>> >> @@ -798,6 +798,7 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
>> >>
>> >>
>> >>  int drm_dev_set_unique(struct drm_device *dev, const char *name);
>> >> +int drm_dev_set_name(struct drm_device *dev, const char *name);
>> >>
>> >>
>> >>  #endif
>> >> --
>> >> 2.17.1
>> >>
>> >> _______________________________________________
>> >> amd-gfx mailing list
>> >> amd-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
> The purpose is to get rid of all PCI ID tables for all drivers in
> userspace. (or at least stop updating them)
>
> Mesa common code and modesetting will use this.

I'd think this would warrant a high level description of what you want
to achieve in the commit message.

BR,
Jani.
Am 17.09.19 um 07:47 schrieb Jani Nikula:
> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
>> The purpose is to get rid of all PCI ID tables for all drivers in
>> userspace. (or at least stop updating them)
>>
>> Mesa common code and modesetting will use this.
> I'd think this would warrant a high level description of what you want
> to achieve in the commit message.

And maybe explicitly call it uapi_name or even uapi_driver_name.

Regards,
Christian.

>
> BR,
> Jani.
>
On Tue, Sep 17, 2019 at 10:12 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 17.09.19 um 07:47 schrieb Jani Nikula:
> > On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
> >> The purpose is to get rid of all PCI ID tables for all drivers in
> >> userspace. (or at least stop updating them)
> >>
> >> Mesa common code and modesetting will use this.
> > I'd think this would warrant a high level description of what you want
> > to achieve in the commit message.
>
> And maybe explicitly call it uapi_name or even uapi_driver_name.

If it's uapi_name, then why do we need a new one for every generation?
Userspace drivers tend to span a lot more than just 1 generation. And
if you want to have per-generation data from the kernel to userspace,
then imo that's much better suited in some amdgpu ioctl, instead of
trying to encode that into the driver name.
-Daniel
Am 17.09.19 um 10:17 schrieb Daniel Vetter:
> On Tue, Sep 17, 2019 at 10:12 AM Christian König

> <ckoenig.leichtzumerken@gmail.com> wrote:

>> Am 17.09.19 um 07:47 schrieb Jani Nikula:

>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:

>>>> The purpose is to get rid of all PCI ID tables for all drivers in

>>>> userspace. (or at least stop updating them)

>>>>

>>>> Mesa common code and modesetting will use this.

>>> I'd think this would warrant a high level description of what you want

>>> to achieve in the commit message.

>> And maybe explicitly call it uapi_name or even uapi_driver_name.

> If it's uapi_name, then why do we need a new one for every generation?

> Userspace drivers tend to span a lot more than just 1 generation. And

> if you want to have per-generation data from the kernel to userspace,

> then imo that's much better suited in some amdgpu ioctl, instead of

> trying to encode that into the driver name.


Well we already have an IOCTL for that, but I thought the intention here 
was to get rid of the PCI-ID tables in userspace to figure out which 
driver to load.

I mean it could be perfectly valid to not only match the kernel, but 
also the hardware generation for that.

Christian.

> -Daniel
On 2019-09-17 10:23 a.m., Koenig, Christian wrote:
> Am 17.09.19 um 10:17 schrieb Daniel Vetter:
>> On Tue, Sep 17, 2019 at 10:12 AM Christian König
>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>> Am 17.09.19 um 07:47 schrieb Jani Nikula:
>>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> The purpose is to get rid of all PCI ID tables for all drivers in
>>>>> userspace. (or at least stop updating them)
>>>>>
>>>>> Mesa common code and modesetting will use this.
>>>> I'd think this would warrant a high level description of what you want
>>>> to achieve in the commit message.
>>> And maybe explicitly call it uapi_name or even uapi_driver_name.
>> If it's uapi_name, then why do we need a new one for every generation?
>> Userspace drivers tend to span a lot more than just 1 generation. And
>> if you want to have per-generation data from the kernel to userspace,
>> then imo that's much better suited in some amdgpu ioctl, instead of
>> trying to encode that into the driver name.
> 
> Well we already have an IOCTL for that, but I thought the intention here 
> was to get rid of the PCI-ID tables in userspace to figure out which 
> driver to load.

That's just unrealistic in general, I'm afraid. See e.g. the ongoing
transition from i965 to iris for recent Intel hardware. How is the
kernel supposed to know which driver is to be used?


For the Xorg modesetting driver, there's a simple solution, see
https://gitlab.freedesktop.org/xorg/xserver/merge_requests/278 .
Something similar can probably be done in Mesa as well.
On 2019-09-17 11:23 a.m., Michel Dänzer wrote:
> On 2019-09-17 10:23 a.m., Koenig, Christian wrote:
>> Am 17.09.19 um 10:17 schrieb Daniel Vetter:
>>> On Tue, Sep 17, 2019 at 10:12 AM Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>> Am 17.09.19 um 07:47 schrieb Jani Nikula:
>>>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> The purpose is to get rid of all PCI ID tables for all drivers in
>>>>>> userspace. (or at least stop updating them)
>>>>>>
>>>>>> Mesa common code and modesetting will use this.
>>>>> I'd think this would warrant a high level description of what you want
>>>>> to achieve in the commit message.
>>>> And maybe explicitly call it uapi_name or even uapi_driver_name.
>>> If it's uapi_name, then why do we need a new one for every generation?
>>> Userspace drivers tend to span a lot more than just 1 generation. And
>>> if you want to have per-generation data from the kernel to userspace,
>>> then imo that's much better suited in some amdgpu ioctl, instead of
>>> trying to encode that into the driver name.
>>
>> Well we already have an IOCTL for that, but I thought the intention here 
>> was to get rid of the PCI-ID tables in userspace to figure out which 
>> driver to load.
> 
> That's just unrealistic in general, I'm afraid. See e.g. the ongoing
> transition from i965 to iris for recent Intel hardware. How is the
> kernel supposed to know which driver is to be used?
> 
> 
> For the Xorg modesetting driver, there's a simple solution, see
> https://gitlab.freedesktop.org/xorg/xserver/merge_requests/278 .
> Something similar can probably be done in Mesa as well.

Another possibility might be for Xorg to use
https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_query_driver.txt
to determine the driver name. Then only Mesa might need any HW specific
code.
Am 17.09.19 um 11:27 schrieb Michel Dänzer:
> On 2019-09-17 11:23 a.m., Michel Dänzer wrote:
>> On 2019-09-17 10:23 a.m., Koenig, Christian wrote:
>>> Am 17.09.19 um 10:17 schrieb Daniel Vetter:
>>>> On Tue, Sep 17, 2019 at 10:12 AM Christian König
>>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>> Am 17.09.19 um 07:47 schrieb Jani Nikula:
>>>>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> The purpose is to get rid of all PCI ID tables for all drivers in
>>>>>>> userspace. (or at least stop updating them)
>>>>>>>
>>>>>>> Mesa common code and modesetting will use this.
>>>>>> I'd think this would warrant a high level description of what you want
>>>>>> to achieve in the commit message.
>>>>> And maybe explicitly call it uapi_name or even uapi_driver_name.
>>>> If it's uapi_name, then why do we need a new one for every generation?
>>>> Userspace drivers tend to span a lot more than just 1 generation. And
>>>> if you want to have per-generation data from the kernel to userspace,
>>>> then imo that's much better suited in some amdgpu ioctl, instead of
>>>> trying to encode that into the driver name.
>>> Well we already have an IOCTL for that, but I thought the intention here
>>> was to get rid of the PCI-ID tables in userspace to figure out which
>>> driver to load.
>> That's just unrealistic in general, I'm afraid. See e.g. the ongoing
>> transition from i965 to iris for recent Intel hardware. How is the
>> kernel supposed to know which driver is to be used?

Well how is userspace currently handling that? The kernel should NOT say 
which driver to use in userspace, but rather which one is used in the 
kernel.

Mapping that information to an userspace driver still needs to be done 
somewhere else, but the difference is that you don't need to add all 
PCI-IDs twice.

Christian.

>>
>>
>> For the Xorg modesetting driver, there's a simple solution, see
>> https://gitlab.freedesktop.org/xorg/xserver/merge_requests/278 .
>> Something similar can probably be done in Mesa as well.
> Another possibility might be for Xorg to use
> https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_query_driver.txt
> to determine the driver name. Then only Mesa might need any HW specific
> code.
>
>
On Tue, Sep 17, 2019 at 11:27 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2019-09-17 11:23 a.m., Michel Dänzer wrote:
> > On 2019-09-17 10:23 a.m., Koenig, Christian wrote:
> >> Am 17.09.19 um 10:17 schrieb Daniel Vetter:
> >>> On Tue, Sep 17, 2019 at 10:12 AM Christian König
> >>> <ckoenig.leichtzumerken@gmail.com> wrote:
> >>>> Am 17.09.19 um 07:47 schrieb Jani Nikula:
> >>>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
> >>>>>> The purpose is to get rid of all PCI ID tables for all drivers in
> >>>>>> userspace. (or at least stop updating them)
> >>>>>>
> >>>>>> Mesa common code and modesetting will use this.
> >>>>> I'd think this would warrant a high level description of what you want
> >>>>> to achieve in the commit message.
> >>>> And maybe explicitly call it uapi_name or even uapi_driver_name.
> >>> If it's uapi_name, then why do we need a new one for every generation?
> >>> Userspace drivers tend to span a lot more than just 1 generation. And
> >>> if you want to have per-generation data from the kernel to userspace,
> >>> then imo that's much better suited in some amdgpu ioctl, instead of
> >>> trying to encode that into the driver name.
> >>
> >> Well we already have an IOCTL for that, but I thought the intention here
> >> was to get rid of the PCI-ID tables in userspace to figure out which
> >> driver to load.
> >
> > That's just unrealistic in general, I'm afraid. See e.g. the ongoing
> > transition from i965 to iris for recent Intel hardware. How is the
> > kernel supposed to know which driver is to be used?
> >
> >
> > For the Xorg modesetting driver, there's a simple solution, see
> > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/278 .
> > Something similar can probably be done in Mesa as well.
>
> Another possibility might be for Xorg to use
> https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_query_driver.txt
> to determine the driver name. Then only Mesa might need any HW specific
> code.

How are other compositors solving this? I don't expect they have a
pciid table like modesetting copied to all of them ...
-Daniel
On 2019-09-17 1:20 p.m., Christian König wrote:
> Am 17.09.19 um 11:27 schrieb Michel Dänzer:
>> On 2019-09-17 11:23 a.m., Michel Dänzer wrote:
>>> On 2019-09-17 10:23 a.m., Koenig, Christian wrote:
>>>> Am 17.09.19 um 10:17 schrieb Daniel Vetter:
>>>>> On Tue, Sep 17, 2019 at 10:12 AM Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>>> Am 17.09.19 um 07:47 schrieb Jani Nikula:
>>>>>>> On Mon, 16 Sep 2019, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>> The purpose is to get rid of all PCI ID tables for all drivers in
>>>>>>>> userspace. (or at least stop updating them)
>>>>>>>>
>>>>>>>> Mesa common code and modesetting will use this.
>>>>>>> I'd think this would warrant a high level description of what you
>>>>>>> want
>>>>>>> to achieve in the commit message.
>>>>>> And maybe explicitly call it uapi_name or even uapi_driver_name.
>>>>> If it's uapi_name, then why do we need a new one for every generation?
>>>>> Userspace drivers tend to span a lot more than just 1 generation. And
>>>>> if you want to have per-generation data from the kernel to userspace,
>>>>> then imo that's much better suited in some amdgpu ioctl, instead of
>>>>> trying to encode that into the driver name.
>>>> Well we already have an IOCTL for that, but I thought the intention
>>>> here
>>>> was to get rid of the PCI-ID tables in userspace to figure out which
>>>> driver to load.
>>> That's just unrealistic in general, I'm afraid. See e.g. the ongoing
>>> transition from i965 to iris for recent Intel hardware. How is the
>>> kernel supposed to know which driver is to be used?
> 
> Well how is userspace currently handling that? The kernel should NOT say
> which driver to use in userspace, but rather which one is used in the
> kernel.

Would that really help though? E.g. the radeon kernel driver supports
radeon/r200/r300/r600/radeonsi DRI drivers, the i915 one i915/i965/iris
(and the amdgpu one radeonsi/amdgpu).

The HW generation identifier proposed in these patches might be useful,
but I suspect there'll always be cases where userspace needs to know
more precisely.


> Mapping that information to an userspace driver still needs to be done
> somewhere else, but the difference is that you don't need to add all
> PCI-IDs twice.

It should only really be necessary in Mesa.


On 2019-09-17 1:32 p.m., Daniel Vetter wrote:
> How are other compositors solving this? I don't expect they have a
> pciid table like modesetting copied to all of them ...

They don't need any of this. The Xorg modesetting driver only did for
determining the client driver name to advertise via the DRI2 extension.


On 2019-09-18 1:41 a.m., Marek Olšák wrote:
> drmVersion::name = amdgpu, radeon, intel, etc.
> drmVersion::desc = vega10, vega12, vega20, ...
> 
> The common Mesa code will use name and desc to select the driver.

Like the Xorg modesetting driver, that code doesn't need this kernel
functionality or new PCI IDs. It can just select the current driver for
all devices which aren't supported by older drivers (which is a fixed
set at this point).


> The AMD-specific Mesa code will use desc to identify the chip.

Doesn't libdrm_amdgpu's struct amdgpu_gpu_info::family_id provide the
same information?