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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 26, 2018 at 10:12:00AM +0000, Jani Nikula wrote:
> On Thu, 25 Jan 2018, Rodrigo Vivi <rodrigo.vivi@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@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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.

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?

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx