Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
On Fri, May 26, 2017 at 3:23 PM, Samuel Li wrote: > From: Xiaojie Yuan > > v2: fix an off by one error and leading white spaces > v3: use thread safe strtok_r(); initialize len before calling getline(); > change printf() to drmMsg(); add initial amdgpu.ids > > Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9 > Reviewed-by: Junwei Zhang > Signed-off-by: Samuel Li > --- > Makefile.am | 3 + > amdgpu/Makefile.am | 2 + > amdgpu/Makefile.sources | 2 +- > amdgpu/amdgpu_asic_id.c | 199 > +++ > amdgpu/amdgpu_asic_id.h | 165 --- > amdgpu/amdgpu_device.c | 28 +-- > amdgpu/amdgpu_internal.h | 10 +++ > include/drm/amdgpu.ids | 154 > 8 files changed, 390 insertions(+), 173 deletions(-) > create mode 100644 amdgpu/amdgpu_asic_id.c > delete mode 100644 amdgpu/amdgpu_asic_id.h > create mode 100644 include/drm/amdgpu.ids > > diff --git a/Makefile.am b/Makefile.am > index dfb8fcd..8de8f6c 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -45,6 +45,9 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \ > > pkgconfigdir = @pkgconfigdir@ > pkgconfig_DATA = libdrm.pc > +libdrmdatadir = $(datadir)/libdrm > +dist_libdrmdata_DATA = include/drm/amdgpu.ids > +export libdrmdatadir > > if HAVE_LIBKMS > LIBKMS_SUBDIR = libkms > diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am > index cf7bc1b..da71c1c 100644 > --- a/amdgpu/Makefile.am > +++ b/amdgpu/Makefile.am > @@ -30,6 +30,8 @@ AM_CFLAGS = \ > $(PTHREADSTUBS_CFLAGS) \ > -I$(top_srcdir)/include/drm > > +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\" > + > libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la > libdrm_amdgpu_ladir = $(libdir) > libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 -no-undefined > diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources > index 487b9e0..bc3abaa 100644 > --- a/amdgpu/Makefile.sources > +++ b/amdgpu/Makefile.sources > @@ -1,5 +1,5 @@ > LIBDRM_AMDGPU_FILES := \ > - amdgpu_asic_id.h \ > + amdgpu_asic_id.c \ > amdgpu_bo.c \ > amdgpu_cs.c \ > amdgpu_device.c \ > diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c > new file mode 100644 > index 000..5b415e3 > --- /dev/null > +++ b/amdgpu/amdgpu_asic_id.c > @@ -0,0 +1,199 @@ > +/* > + * Copyright © 2017 Advanced Micro Devices, Inc. > + * All Rights Reserved. > + * > + * 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 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 > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) 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. > + * > + */ > + > +#ifdef HAVE_CONFIG_H > +#include "config.h" > +#endif > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "xf86drm.h" > +#include "amdgpu_drm.h" > +#include "amdgpu_internal.h" > + > +static int parse_one_line(const char *line, struct amdgpu_asic_id *id) > +{ > + char *buf, *saveptr; > + char *s_did; > + char *s_rid; > + char *s_name; > + char *endptr; > + int r = 0; > + > + buf = strdup(line); > + if (!buf) > + return -ENOMEM; > + > + /* ignore empty line and commented line */ > + if (strlen(line) == 0 || line[0] == '#') { > + r = -EAGAIN; > + goto out; > + } > + > + /* device id */ > + s_did = strtok_r(buf, ",", &saveptr); > + if (!s_did) { > + r = -EINVAL; > + goto out; > + } > + > + id->did = strtol(s_did, &endptr, 16); > + if (*endptr) { > + r = -EINVAL; > + goto out; > + } > + > + /* revision id */ > + s_rid = strtok_r(NULL, ",", &saveptr); > + if (!s_rid) { > + r = -EINVAL; > + goto out; > + } > + > + id->rid = strtol(s_rid, &endptr, 16); > + if (*endptr) { > + r = -EINVAL; > + goto out; > + } > + > +
Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
On 27/05/17 04:23 AM, Samuel Li wrote: > > diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids > new file mode 100644 > index 000..1b00b60 > --- /dev/null > +++ b/include/drm/amdgpu.ids > @@ -0,0 +1,154 @@ > +1.0.0 > +6600,0,AMD Radeon HD 8600/8700M This doesn't look like the current format we've settled on internally. There are supposed to be tabs after the commas to align the columns. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
Understood your point. However as discussed internally before, marketing names are there for a lot of reasons; my understanding of the policy is we do not need to touch them as long as there is no error in the names and they are allowed to be public. Regards, Sam -Original Message- From: Alex Deucher [mailto:alexdeuc...@gmail.com] Sent: Friday, May 26, 2017 3:27 PM To: Li, Samuel Cc: amd-gfx list ; Yuan, Xiaojie Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file On Fri, May 26, 2017 at 3:23 PM, Samuel Li wrote: > From: Xiaojie Yuan > > v2: fix an off by one error and leading white spaces > v3: use thread safe strtok_r(); initialize len before calling getline(); > change printf() to drmMsg(); add initial amdgpu.ids > > Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9 > Reviewed-by: Junwei Zhang > Signed-off-by: Samuel Li > --- > Makefile.am | 3 + > amdgpu/Makefile.am | 2 + > amdgpu/Makefile.sources | 2 +- > amdgpu/amdgpu_asic_id.c | 199 > +++ > amdgpu/amdgpu_asic_id.h | 165 --- > amdgpu/amdgpu_device.c | 28 +-- > amdgpu/amdgpu_internal.h | 10 +++ > include/drm/amdgpu.ids | 154 > 8 files changed, 390 insertions(+), 173 deletions(-) create mode > 100644 amdgpu/amdgpu_asic_id.c delete mode 100644 > amdgpu/amdgpu_asic_id.h create mode 100644 include/drm/amdgpu.ids > > diff --git a/Makefile.am b/Makefile.am index dfb8fcd..8de8f6c 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -45,6 +45,9 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \ > > pkgconfigdir = @pkgconfigdir@ > pkgconfig_DATA = libdrm.pc > +libdrmdatadir = $(datadir)/libdrm > +dist_libdrmdata_DATA = include/drm/amdgpu.ids export libdrmdatadir > > if HAVE_LIBKMS > LIBKMS_SUBDIR = libkms > diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index > cf7bc1b..da71c1c 100644 > --- a/amdgpu/Makefile.am > +++ b/amdgpu/Makefile.am > @@ -30,6 +30,8 @@ AM_CFLAGS = \ > $(PTHREADSTUBS_CFLAGS) \ > -I$(top_srcdir)/include/drm > > +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\" > + > libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la libdrm_amdgpu_ladir > = $(libdir) libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 > -no-undefined diff --git a/amdgpu/Makefile.sources > b/amdgpu/Makefile.sources index 487b9e0..bc3abaa 100644 > --- a/amdgpu/Makefile.sources > +++ b/amdgpu/Makefile.sources > @@ -1,5 +1,5 @@ > LIBDRM_AMDGPU_FILES := \ > - amdgpu_asic_id.h \ > + amdgpu_asic_id.c \ > amdgpu_bo.c \ > amdgpu_cs.c \ > amdgpu_device.c \ > diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new > file mode 100644 index 000..5b415e3 > --- /dev/null > +++ b/amdgpu/amdgpu_asic_id.c > @@ -0,0 +1,199 @@ > +/* > + * Copyright © 2017 Advanced Micro Devices, Inc. > + * All Rights Reserved. > + * > + * 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 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 > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) 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. > + * > + */ > + > +#ifdef HAVE_CONFIG_H > +#include "config.h" > +#endif > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "xf86drm.h" > +#include "amdgpu_drm.h" > +#include "amdgpu_internal.h" > + > +static int parse_one_line(const char *line, struct amdgpu_asic_id > +*id) { > + char *buf, *saveptr; > + char *s_did; > + char *s_rid; &
RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
Recently there are some changes internally, I can integrate them into this one. There are still discussions on going regarding the format. Sam -Original Message- From: Michel Dänzer [mailto:mic...@daenzer.net] Sent: Sunday, May 28, 2017 10:11 PM To: Li, Samuel Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file On 27/05/17 04:23 AM, Samuel Li wrote: > > diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new file > mode 100644 index 000..1b00b60 > --- /dev/null > +++ b/include/drm/amdgpu.ids > @@ -0,0 +1,154 @@ > +1.0.0 > +6600,0,AMD Radeon HD 8600/8700M This doesn't look like the current format we've settled on internally. There are supposed to be tabs after the commas to align the columns. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
Hi all, Pardon for dropping in uninvited. Just some food for thought. On 29 May 2017 at 22:01, Li, Samuel wrote: > Understood your point. However as discussed internally before, marketing > names are there for a lot of reasons; my understanding of the policy is we do > not need to touch them as long as there is no error in the names and they are > allowed to be public. > Samuel, It seems that most comments put forward by people are going on deaf ears. While there may be valid arguments behind doing so, do consider the following: - Review is always encouraged Regardless if the information is within or outside of the source code. - Marketing can make mistakes or have IT glitches The inconsistent use of "(TM)" and using a 67C2:00 is something one wants to double-check with them. - Having a separate file so that clients can update/edit it does not help much. You want to ship the whole driver, in order to have a predictable and consistent user experience. - Adding ~200 loc for ~170 devices entries sounds like a step in the wrong direction. In either case, not my call. I might follow-up with some issues in the code itself ;-) HTH Emil ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
> - Marketing can make mistakes or have IT glitches The inconsistent use of > "(TM)" and using a 67C2:00 is something one wants to double-check with them. Marketing names are there for a lot of reasons. The code here is to pass the names only. If you are interested in a vendor's marketing names, please reach out to the vendor, e.g. through your contacts in the vendor who also shares your interest. >- Having a separate file so that clients can update/edit it does not help >much. Please say it to pci.ids/usb.ids :) > Adding ~200 loc for ~170 devices entries sounds like a step in the wrong > direction. Check the vendor's entries in pci.ids, and you might have some better idea. Sam -Original Message- From: Emil Velikov [mailto:emil.l.veli...@gmail.com] Sent: Tuesday, May 30, 2017 7:01 AM To: Li, Samuel Cc: Alex Deucher ; amd-gfx list ; Yuan, Xiaojie Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file Hi all, Pardon for dropping in uninvited. Just some food for thought. On 29 May 2017 at 22:01, Li, Samuel wrote: > Understood your point. However as discussed internally before, marketing > names are there for a lot of reasons; my understanding of the policy is we do > not need to touch them as long as there is no error in the names and they are > allowed to be public. > Samuel, It seems that most comments put forward by people are going on deaf ears. While there may be valid arguments behind doing so, do consider the following: - Review is always encouraged Regardless if the information is within or outside of the source code. - Marketing can make mistakes or have IT glitches The inconsistent use of "(TM)" and using a 67C2:00 is something one wants to double-check with them. - Having a separate file so that clients can update/edit it does not help much. You want to ship the whole driver, in order to have a predictable and consistent user experience. - Adding ~200 loc for ~170 devices entries sounds like a step in the wrong direction. In either case, not my call. I might follow-up with some issues in the code itself ;-) HTH Emil ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
On 30 May 2017 at 22:59, Li, Samuel wrote: >> - Marketing can make mistakes or have IT glitches The inconsistent use of >> "(TM)" and using a 67C2:00 is something one wants to double-check with them. > Marketing names are there for a lot of reasons. The code here is to pass the > names only. > If you are interested in a vendor's marketing names, please reach out to the > vendor, e.g. through your contacts in the vendor who also shares your > interest. > True, yet orthogonal to what I'm saying. As people point potential mistakes you (perhaps not personally) want to check if those are genuine or not. If things are correct, say 67C2:00 is valid, simply mention "yes A/B is bit unusual, yet it's the correct name". >>- Having a separate file so that clients can update/edit it does not help >>much. > Please say it to pci.ids/usb.ids :) > Those files have many more users than the amdgpu.ids, have existed for ~20 years. Since you refer to pci.ids - why not reuse it but grow a local copy instead? >> Adding ~200 loc for ~170 devices entries sounds like a step in the wrong >> direction. > Check the vendor's entries in pci.ids, and you might have some better idea. > The file lists ~2.8k entries for 1002 ATI/AMD, yet those include north bridges and others which are not applicable here. I'm afraid your argument is too subtle. As they say "Not my circus not my moneys" (don't take this the wrong way). Regards, Emil P.S. Please convince your email client to quote properly? ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file
On Wed, May 31, 2017 at 7:19 AM, Emil Velikov wrote: > On 30 May 2017 at 22:59, Li, Samuel wrote: >>> - Marketing can make mistakes or have IT glitches The inconsistent use of >>> "(TM)" and using a 67C2:00 is something one wants to double-check with them. >> Marketing names are there for a lot of reasons. The code here is to pass the >> names only. >> If you are interested in a vendor's marketing names, please reach out to the >> vendor, e.g. through your contacts in the vendor who also shares your >> interest. >> > True, yet orthogonal to what I'm saying. As people point potential > mistakes you (perhaps not personally) want to check if those are > genuine or not. > If things are correct, say 67C2:00 is valid, simply mention "yes A/B > is bit unusual, yet it's the correct name". > >>>- Having a separate file so that clients can update/edit it does not help >>>much. >> Please say it to pci.ids/usb.ids :) >> > Those files have many more users than the amdgpu.ids, have existed for > ~20 years. Since you refer to pci.ids - why not reuse it but grow a > local copy instead? The problem with pci.ids is that it doesn't take into account pci revisions so it would be comparable work either way. Alex > >>> Adding ~200 loc for ~170 devices entries sounds like a step in the wrong >>> direction. >> Check the vendor's entries in pci.ids, and you might have some better idea. >> > The file lists ~2.8k entries for 1002 ATI/AMD, yet those include north > bridges and others which are not applicable here. > I'm afraid your argument is too subtle. > > As they say "Not my circus not my moneys" (don't take this the wrong way). > > Regards, > Emil > P.S. Please convince your email client to quote properly? ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx