[Intel-gfx] [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

Thu Aug 31 07:59:16 UTC 2017

On Wed, Aug 30, 2017 at 09:40:06PM -0700, Rodrigo Vivi wrote:
> On Wed, Aug 30, 2017 at 7:58 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan at intel.com> wrote:
> > On Wed, 2017-08-30 at 20:32 +0100, Chris Wilson wrote:
> >> Quoting Pandiyan, Dhinakaran (2017-08-30 20:12:52)
> >> > On Wed, 2017-08-16 at 17:54 -0700, Rodrigo Vivi wrote:
> >> > > Let's make it easier to add platforms that supports 2 pixel per
> >> > > clock.
> >> > >
> >> > > With spread checks per platform it was easy to miss one or
> >> > > another spot leading to loose some time on debug.
> >> > >
> >> > > Hopefully this check would save some cases in the future.
> >> > >
> >> > > No functional change.
> >> > >
> >> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> >> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
> >> > >  drivers/gpu/drm/i915/i915_pci.c    | 2 ++
> >> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> >> > >  drivers/gpu/drm/i915/intel_pm.c    | 3 +--
> >> > >  4 files changed, 11 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > > index 6c25c8520c87..94f5e6522e5e 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > > @@ -748,6 +748,7 @@ struct intel_csr {
> >> > >       func(is_lp); \
> >> > >       func(is_alpha_support); \
> >> > >       /* Keep has_* in alphabetical order */ \
> >> > > +     func(has_2ppc); \
> >> > >       func(has_64bit_reloc); \
> >> > >       func(has_aliasing_ppgtt); \
> >> > >       func(has_csr); \
> >> > > @@ -3025,6 +3026,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> >> > >  #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> >> > >       (IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> >> > >
> >> > > +/* Supports 2 pixel per clock */
> >> > > +#define HAS_2PPC(dev_priv) ((dev_priv)->info.has_2ppc)
> >> > > +
> >> >
> >> > How about
> >> > #define HAS_2PPC(dev_priv) (IS_GEMINILAKE(dev_priv) ||
> >> > INTEL_GEN(dev_priv) >= 10) ?
> >> >
> >> > I am not clear on what qualifies for a place in device_info, but
> >> > defining it this way let's me go to the definition and quickly check
> >> > which platform has 2 pixels per clock.
> 
> One thing I always wanted was to avoid 2 places to declare features &
> capabilities.
> Here or on the platform.

So should we do a bit of extraction to bring the two closer together? I.e.
h/c file with only the device info and feature macros, nothing else. We
still need the split, but well we need the structure in a header anyway.

If you actually want to bring them closer together, then you need to bring
them closer together. Not arbitrarily prefer one over the other (while
still having piles of other users around of the non-preferred version).
> 
> >>
> >> A couple of rules of thumb for starting with:
> >>
> >> Use device_info if:
> >>
> >>  - it fundamentally changes how the device operates, such that knowing
> >>    about it in debug logs is a key means of triage
> 
> I think on this one 2ppc qualifies

gen3 did double-clock too, just fyi :-) We survived just fine without a
feature flag on that.

Anyway, just my bikeshed.

> >>
> >>  - number of branches x callsites > 8
> 
> 2 * 5 > 8 in this case?
> or I'm getting the number of "branches" incorrectly?
> 
> What I was trying to save as well is the addition of any next platform.
> When adding the feature it would be easier to search for HAS_2PPC instead of
> searching for glk or cnl and analising that individually to see if it
> is 2pp before
>  see if it applies to that platform.
> 
> But the reason that I put this patch on top is that I don't have
> strong opinion here so
> t would be fine for me to move on without ht.

HAS_2PPC definitely makes sense, but I'm not sure it makes sense to stuff
everything into intel_device_info. Going that way some people might start
to think that this is the complete solution to describing everything, and
that definitely doesn't work.
-Daniel

> Thanks,
> Rodrigo.
> 
> >>    (some estimate of the cost of inclusion inside device_info vs
> >>    savings in object code, for a more realistic estimate a branch will
> >>    ~12 bytes (depending on the phase of the moon) and cost for device
> >>    info will be the addition of a few strings, and a couple of calls
> >>    to use those string, so at a guess 100 bytes.)
> >
> > That's an interesting back-of-the-envelope calculation. I suppose the
> > compiler is also more likely to load device_info->gen into a register
> > than device_info->has_2ppc.
> >
> >
> >>
> >> -Chris
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch