Re: [PATCH] drm/i915: Separate cherryview from valleyview

[Daniel Vetter <daniel@ffwll.ch>]

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@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.
What would that look like (maybe as patch series)?

And if it's prep work for future platforms, the patch series should
mention that (without mentioning the platform name/gen ofc). I usually
know, but sometimes it's hard to guess ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx