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

[Jani Nikula]

On Mon, 29 Jan 2018, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan at intel.com> wrote:
> On Sat, 2018-01-27 at 11:05 +0200, 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.
>> 
>
> I am wondering if we can augment the device info struct at run-time with
> a union of platform-specific key-value pairs.

That occurred to me too, but I'm not thrilled about the prospect of
maintaining the union and ensuring we only ever access the fields within
correct platform specific code or conditions.

BR,
Jani.


>
>
>> > 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. ;)
>> 
>> BR,
>> Jani.
>> 
>> >
>> > on this particular case here I considered that, but
>> > since I had no good name and INTEL_DEVID was there I've
>> > chosen the lazy path.
>> >
>> >> 
>> >> Okay, so this is late in the review cycles, and part of a more general
>> >> problem, so we should probably just go with this for now and come back
>> >> to this later.
>> >
>> > It is never too late ;)
>> >
>> > But in this case I'd prefer moving with this series as is
>> > to avoid blocking Paulo with ICL enabling and minimize
>> > the conflicts and rebase pain to only one area of the code.
>> >
>> > Thanks for bringing it up,
>> > Rodrigo.
>> >
>> >> 
>> >> 
>> >> BR,
>> >> Jani.
>> >> 
>> >> 
>> >> 
>> >> 
>> >> >  
>> >> >  #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>> >> >  
>> >> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> >> > index f28c165fc49d..7eb3d5e4350e 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_pci.c
>> >> > +++ b/drivers/gpu/drm/i915/i915_pci.c
>> >> > @@ -571,7 +571,7 @@ static const struct intel_device_info intel_coffeelake_gt3_info __initconst = {
>> >> >  	.ddb_size = 1024, \
>> >> >  	GLK_COLORS
>> >> >  
>> >> > -static const struct intel_device_info intel_cannonlake_gt2_info __initconst = {
>> >> > +static const struct intel_device_info intel_cannonlake_info __initconst = {
>> >> >  	GEN10_FEATURES,
>> >> >  	.is_alpha_support = 1,
>> >> >  	.platform = INTEL_CANNONLAKE,
>> >> > @@ -649,8 +649,7 @@ static const struct pci_device_id pciidlist[] = {
>> >> >  	INTEL_CFL_U_GT1_IDS(&intel_coffeelake_gt1_info),
>> >> >  	INTEL_CFL_U_GT2_IDS(&intel_coffeelake_gt2_info),
>> >> >  	INTEL_CFL_U_GT3_IDS(&intel_coffeelake_gt3_info),
>> >> > -	INTEL_CNL_U_GT2_IDS(&intel_cannonlake_gt2_info),
>> >> > -	INTEL_CNL_Y_GT2_IDS(&intel_cannonlake_gt2_info),
>> >> > +	INTEL_CNL_IDS(&intel_cannonlake_info),
>> >> >  	{0, 0, 0}
>> >> >  };
>> >> >  MODULE_DEVICE_TABLE(pci, pciidlist);
>> >> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
>> >> > index 5db0458dd832..9e1fe6634424 100644
>> >> > --- a/include/drm/i915_pciids.h
>> >> > +++ b/include/drm/i915_pciids.h
>> >> > @@ -414,24 +414,20 @@
>> >> >  	INTEL_CFL_U_GT2_IDS(info), \
>> >> >  	INTEL_CFL_U_GT3_IDS(info)
>> >> >  
>> >> > -/* CNL U 2+2 */
>> >> > -#define INTEL_CNL_U_GT2_IDS(info) \
>> >> > +/* CNL */
>> >> > +#define INTEL_CNL_IDS(info) \
>> >> >  	INTEL_VGA_DEVICE(0x5A52, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A5A, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A42, info), \
>> >> > -	INTEL_VGA_DEVICE(0x5A4A, info)
>> >> > -
>> >> > -/* CNL Y 2+2 */
>> >> > -#define INTEL_CNL_Y_GT2_IDS(info) \
>> >> > +	INTEL_VGA_DEVICE(0x5A4A, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A51, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A59, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A41, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A49, info), \
>> >> >  	INTEL_VGA_DEVICE(0x5A71, info), \
>> >> > -	INTEL_VGA_DEVICE(0x5A79, info)
>> >> > -
>> >> > -#define INTEL_CNL_IDS(info) \
>> >> > -	INTEL_CNL_U_GT2_IDS(info), \
>> >> > -	INTEL_CNL_Y_GT2_IDS(info)
>> >> > +	INTEL_VGA_DEVICE(0x5A79, info), \
>> >> > +	INTEL_VGA_DEVICE(0x5A54, info), \
>> >> > +	INTEL_VGA_DEVICE(0x5A5C, info), \
>> >> > +	INTEL_VGA_DEVICE(0x5A44, info)
>> >> >  
>> >> >  #endif /* _I915_PCIIDS_H */
>> >> 
>> >> -- 
>> >> Jani Nikula, Intel Open Source Technology Center
>> 

-- 
Jani Nikula, Intel Open Source Technology Center