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

Mon Jan 29 19:09:04 UTC 2018

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.

> > 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
> 