Re: [PATCH 2/2] drm/i915: Introduce HAS_2PPC.

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Wed, Aug 30, 2017 at 7:58 PM, Pandiyan, Dhinakaran
<dhinakaran.pandiyan@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@intel.com>
>> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

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

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

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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx