[Intel-gfx] [PATCH] drm/i915: Separate cherryview from valleyview

[Ville Syrjälä]

On Fri, Dec 04, 2015 at 05:51:56PM +0100, Daniel Vetter wrote:
> On Fri, Dec 04, 2015 at 04:15:27PM +0000, Vivi, Rodrigo wrote:
> > On Fri, 2015-12-04 at 16:05 +0100, Daniel Vetter wrote:
> > > On Wed, Dec 02, 2015 at 02:30:28PM +0200, Jani Nikula wrote:
> > > > On Wed, 02 Dec 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> 
> > > > wrote:
> > > > > On Tue, Dec 01, 2015 at 05:11:40PM -0800, Wayne Boyer wrote:
> > > > > > The cherryview device shares many characteristics with the 
> > > > > > valleyview
> > > > > > device.  When support was added to the driver for cherryview, 
> > > > > > the
> > > > > > corresponding device info structure included .is_valleyview = 
> > > > > > 1.
> > > > > > This is not correct and leads to some confusion.
> > > > > > 
> > > > > > This patch changes .is_valleyview to .is_cherryview in the 
> > > > > > cherryview
> > > > > > device info structure and defines the HAS_GEN7_LP_FEATURES 
> > > > > > macro.
> > > > > > Then where appropriate, instances of IS_VALLEYVIEW are replaced 
> > > > > > with
> > > > > > HAS_GEN7_LP_FEATURES to test for either a valleyview or a 
> > > > > > cherryview
> > > > > > device.
> > > > > 
> > > > > I don't like the name of the macro. Most of the shared bits are 
> > > > > display
> > > > > related, so we could have something like HAS_VLV_DISPLAY. For the 
> > > > > rest,
> > > > > I think we could just test IS_VLV||IS_CHV explicitly. Unless 
> > > > > someone
> > > > > can come up with a better name that covers everything?
> > > > 
> > > > Definitely NAK on HAS_GEN7_LP_FEATURES.
> > > > 
> > > > HAS_VLV_DISPLAY would be a subset of HAS_GMCH_DISPLAY, which I 
> > > > guess
> > > > wouldn't be that bad... unless someone starts using that for a 
> > > > VLV||CHV
> > > > shorthand in non-display code.
> > > > 
> > > > I think I might just go for the verbose (IS_VALLEYVIEW || 
> > > > IS_CHERRYVIEW)
> > > > all around. Especially since we've been brainwashed with the old 
> > > > vlv is
> > > > both vlv and chv code.
> > > 
> > > HAS_GMCH_DISPLAY is what I've generally used, since usually you have 
> > > a
> > > INTEL_INFO()->gen >= 5 test already somewhere. If we want to make 
> > > this
> > > more explicit the proper name for vlv is BAYTRAIL, and for truely byt
> > > specific stuff we've named things byt_. So what about Defining an
> > > IS_BAYTRAIL instead for the cases where it's not vlv || chv.
> > 
> > Baytrail is the platform name with the Valleyview graphics. Than we
> > would have Cherry Trail and/or Braswell for Cherryview graphics and
> > Apollo Lake for Broxton. So I would vote to stay with Valleyview,
> > Cherryview and Broxton only.
> > 
> > > 
> > > And what's the benefit of all this churn?
> > 
> > Organize and prepare the code for future platforms. 
> > Avoid more confusion like we had on IS_SKYLAKE x IS_KABYLAKE.
> > Make things more easy and clear if we decide to add .is_atom_lp on
> > these platforms definition.
> 
> .is_atom_lp is imo the more sensible change to do, since it includes bxt.

BXT vs. VLV/CHV have practically nothing in common in the driver,
so I wouldn't go there.

-- 
Ville Syrjälä
Intel OTC