Re: [PATCH i-g-t] lib: sync PCI ID macros with kernel

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 12 Nov 2024, Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> wrote:
> On Tue, Nov 12, 2024 at 01:22:47PM +0200, Jani Nikula wrote:
>> On Tue, 12 Nov 2024, Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> wrote:
>> > On Thu, Nov 07, 2024 at 10:49:30PM +0200, Jani Nikula wrote:
>> >> On Thu, 07 Nov 2024, Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:
>> >> > On 2024-11-06 at 10:58:46 -0800, Ngai-Mint Kwan wrote:
>> >> >> diff --git a/lib/i915_pciids.h b/lib/pciids.h
>> >> >> similarity index 93%
>> >> >> rename from lib/i915_pciids.h
>> >> >> rename to lib/pciids.h
>> >> >> index 3e39d644e..7632507af 100644
>> >> >> --- a/lib/i915_pciids.h
>> >> >> +++ b/lib/pciids.h
>> >> >> @@ -22,30 +22,23 @@
>> >> >>   * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> >> >>   * DEALINGS IN THE SOFTWARE.
>> >> >>   */
>> >> >> -#ifndef _I915_PCIIDS_H
>> >> >> -#define _I915_PCIIDS_H
>> >> >> -
>> >> >> -/*
>> >> >> - * A pci_device_id struct {
>> >> >> - *	__u32 vendor, device;
>> >> >> - *      __u32 subvendor, subdevice;
>> >> >> - *	__u32 class, class_mask;
>> >> >> - *	kernel_ulong_t driver_data;
>> >> >> - * };
>> >> >> - * Don't use C99 here because "class" is reserved and we want to
>> >> >> - * give userspace flexibility.
>> >> >> - */
>> >> >> -#define INTEL_VGA_DEVICE(id, info) { \
>> >> >> -	0x8086,	id, \
>> >> >> -	~0, ~0, \
>> >> >> -	0x030000, 0xff0000, \
>> >> >> -	(unsigned long) info }
>> >> >> -
>> >> >> -#define INTEL_QUANTA_VGA_DEVICE(info) { \
>> >> >> -	0x8086,	0x16a, \
>> >> >> -	0x152d,	0x8990, \
>> >> >> -	0x030000, 0xff0000, \
>> >> >> -	(unsigned long) info }
>> >> >> +#ifndef __PCIIDS_H__
>> >> >> +#define __PCIIDS_H__
>> >> >> +
>> >> >> +#ifdef __KERNEL__
>> >> >
>> >> > I am not sure if we want a kernel defs, this is for userspace
>> >> > so imho this ifdef KERNEL/endif should be deleted. Or write
>> >> > a rationale why you included it here, or make it a comment?
>> >> > +cc Zbigniew
>> >> 
>> >> The file comes verbatim from kernel.
>> >
>> > I have mixed feelings about copying kernel headers here directly
>> > if they are not uapi. However __KERNEL__ conditional was added to
>> > this header intentionally so copying this file outside the kernel
>> > and further reuse in userspace code is harmless. But I would add
>> > some explanation to README.md about pciids.h copying procedure
>> > for the future.
>> 
>> We've been doing this for PCI IDs for at least 10 years, and for
>> intel_vbt_defs.h for at least 7 years.
>> 
>> Please feel free to send documentation patches.
>
> I see no __KERNEL__ in current xe_pciids.h and i915_pciids.h so
> there's some change from my perspective. intel_vbt_defs.h also
> doesn't contain any kernel conditional inside. I think author
> of this change should provide additional documentation about this
> (not me) - commit message for me is not enough. Till this change
> I haven't noticed any __KERNEL__ usage in igt codebase and I think
> if it appears it should be documented what's for dead code is
> imported to the project.

See igt commit e966143f5c5f ("lib/intel_device_info: use dedicated macro
for struct pci_id_match init"). INTEL_VGA_DEVICE has been unused in igt
since then.

I wrapped the macro in #ifdef __KERNEL__ in kernel commit fc9cb46bdca8
("drm/i915/pciids: use designated initializers in INTEL_VGA_DEVICE()")
to avoid it being used in igt again.

I'm not going to document this further in igt.


BR,
Jani.


-- 
Jani Nikula, Intel