[Intel-gfx] [PATCH 01/10] drm/i915/cnl: Add Cannonlake PCI IDs for another SKU.

[Tvrtko Ursulin]

On 27/01/2018 09:05, Jani Nikula wrote:
> On Fri, 26 Jan 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>> On Fri, Jan 26, 2018 at 10:12:00AM +0000, Jani Nikula wrote:
>>> On Thu, 25 Jan 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>>>> The only difference is that this SKUs has the full
>>>> Port A/E split named as Port F.
>>>>
>>>> But since SKUs differences don't matter on the platform
>>>> definition group and ids, let's merge all off them together.
>>>>
>>>> v2: Really include the PCI IDs to the picidlist[];
>>>> v3: Add the PCI Id for another SKU (Anusha).
>>>> v4: Update IDs, really include to pciidlists again.
>>>> v5: Unify all GT2 IDs.
>>>> v6: Unify in a way that we don't break early-quirks.c
>>>> v7: Remove GT reference since it doesn't matter here (Paulo)
>>>>      Also move IS_CNL_WITH_PORT_F macro to this patch to
>>>>      make it easier for review this part and also to get
>>>>      used sooner.
>>>>
>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h |  2 ++
>>>>   drivers/gpu/drm/i915/i915_pci.c |  5 ++---
>>>>   include/drm/i915_pciids.h       | 18 +++++++-----------
>>>>   3 files changed, 11 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 454d8f937fae..5702ebf17974 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2648,6 +2648,8 @@ intel_info(const struct drm_i915_private *dev_priv)
>>>>   				 (dev_priv)->info.gt == 2)
>>>>   #define IS_CFL_GT3(dev_priv)	(IS_COFFEELAKE(dev_priv) && \
>>>>   				 (dev_priv)->info.gt == 3)
>>>> +#define IS_CNL_WITH_PORT_F(dev_priv)   (IS_CANNONLAKE(dev_priv) && \
>>>> +					(INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)
>>>
>>> I'd be happy if bit 2 in device id actually meant "port F", but I'm not
>>> so happy with coming up with rules like this for coincidental things.
>>
>> the port F thing is just that we have no name for that SKU :(
>> but from the spec organization this bit seems to represent that
>> group. Although I fully agree with you that this is horrible.
>>
>>>
>>> More generally, I'm not all that happy about *any* of the INTEL_DEVID
>>> uses in i915_drv.h because it spreads out the device id information, so
>>> I'd rather not add more. I'd rather move towards single point of truth
>>> for device ids.
>>
>> Yeah. I guess I agree with you.
>> There should be something inside the device info right?
>> even if we have to separate that in two groups.
> 
> That's the thing. It also worries me to add everything in device info,
> because it grows the device info structs for all devices, and you also
> need to add more device infos to cover all combinations. Unless you
> initialize them runtime, which is also a bit meh.
> 
>> If you agree with this approach I can follow up with a patch/series
>> that kill INTEL_DEVID in favor of device info structs.
>>
>> Or do you have any other idea for solving this?
> 
> Not really. Cc: Chris and Tvrtko who seem to come up with ideas for
> things like this on a regular basis. ;)

If I understood the gist, if single point of truth for device ids is the 
desire, that seems to imply there has to some place where flags are 
toggled at runtime.

Getting rid of INTEL_DEVID checks sprinkled around randomly, as courtesy 
of i915_drv.h "is-platform-variant" macros would be nice, I agree.

So perhaps a runtime pass which would set a new field in device_info, 
like INTEL_INFO()->platform_variant wouldn't be that bad. Device ID 
checks would then be moved from i915_drv.h into intel_device_info.c, and 
the i915_drv.h macros would become simple checks.

Wrt size consideration, I am not sure if it would be feasible to split 
struct intel_device_info in const friendly part and the rest - so that 
the instances in i915_pci.c could be smaller, and copied over to 
larger/complete version in struct drm_i915_private. On a quick glance it 
doesn't seem that there are many, if any, fields which are set at 
runtime only.

So no magical ideas from me I'm afraid.

Regards,

Tvrtko