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

["Boyer, Wayne" <wayne.boyer@intel.com>]

On 12/4/15, 9:24 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> wrote:


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

Thanks for the reviews and comments.  I'll submit a new series that
incorporates the updates and related fixes.

Wayne Boyer

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx