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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 04, 2015 at 05:14:19PM +0000, Vivi, Rodrigo wrote:
> On Fri, 2015-12-04 at 19:04 +0200, Ville Syrjälä wrote:
> > 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@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.
> 
> this was exactly the point where HAS_GEN7_LP_FEATURES appeared,

Which is confusing since since CHV is gen8. And all the features that
are shared have nothing to do with the GT block which is what the gen
numbers identify.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx