[Intel-gfx] [PATCH] drm/i915: stop storing the media fuse

[Michal Wajdeczko]

On Fri, 22 Mar 2019 09:29:58 +0100, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Daniele Ceraolo Spurio (2019-03-22 00:24:31)
>> We're already updating the engine_mask to reflect what's in the HW, so
>> we can just get the info from there. A couple of macros have been added
>> to facilitate this.
>>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
>>  drivers/gpu/drm/i915/intel_device_info.c | 15 ++++++++-------
>>  drivers/gpu/drm/i915/intel_device_info.h |  4 ----
>>  3 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index fefcb39aefc4..9d8d423641d2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2440,6 +2440,12 @@ static inline unsigned int  
>> i915_sg_segment_size(void)
>>  #define ALL_ENGINES    (~0u)
>>  #define HAS_ENGINE(dev_priv, id) (INTEL_INFO(dev_priv)->engine_mask &  
>> BIT(id))
>>
>> +#define ENGINE_INSTANCES_MASK(dev_priv, first, count) \
>> +       ((INTEL_INFO(dev_priv)->engine_mask & \
>> +         GENMASK(first + count - 1, first)) >> first)
>
> Checkpatch complains if we don't wrap everything (inside).
>
> We could even go so far as
>
> ({
> 	int first__ = (first);
> 	int count__ = (count);
> 	INTEL_INFO(i915)-engine_mask & GENMASK(first__ + count__ - 1, first__)  
> >> first__;
> })
>
> gcc should be smart enough to constant fold that away.
>
>> +#define VDBOX_MASK(dev_priv) ENGINE_INSTANCES_MASK(dev_priv, VCS0,  
>> I915_MAX_VCS);
>> +#define VEBOX_MASK(dev_priv) ENGINE_INSTANCES_MASK(dev_priv, VECS0,  
>> I915_MAX_VECS);
>> +
>>  #define HAS_LLC(dev_priv)      (INTEL_INFO(dev_priv)->has_llc)
>>  #define HAS_SNOOP(dev_priv)    (INTEL_INFO(dev_priv)->has_snoop)
>>  #define HAS_EDRAM(dev_priv)    (!!((dev_priv)->edram_cap &  
>> EDRAM_ENABLED))
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c  
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index eddf83807957..74efabd12351 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -871,22 +871,23 @@ void intel_device_info_init_mmio(struct  
>> drm_i915_private *dev_priv)
>>         unsigned int logical_vdbox = 0;
>>         unsigned int i;
>>         u32 media_fuse;
>> +       u16 vdbox_mask, vebox_mask;
>
> I would put these on separate lines, just to keep the vertical
> whitespace clean.
>
>>         if (INTEL_GEN(dev_priv) < 11)
>>                 return;
>>
>>         media_fuse = ~I915_READ(GEN11_GT_VEBOX_VDBOX_DISABLE);
>>
>> -       RUNTIME_INFO(dev_priv)->vdbox_enable = media_fuse &  
>> GEN11_GT_VDBOX_DISABLE_MASK;
>> -       RUNTIME_INFO(dev_priv)->vebox_enable = (media_fuse &  
>> GEN11_GT_VEBOX_DISABLE_MASK) >>
>> -               GEN11_GT_VEBOX_DISABLE_SHIFT;
>> +       vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
>> +       vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
>> +                     GEN11_GT_VEBOX_DISABLE_SHIFT;
>>
>> -       DRM_DEBUG_DRIVER("vdbox enable: %04x\n",  
>> RUNTIME_INFO(dev_priv)->vdbox_enable);
>> +       DRM_DEBUG_DRIVER("vdbox enable: %04x\n", vdbox_mask);
>>         for (i = 0; i < I915_MAX_VCS; i++) {
>>                 if (!HAS_ENGINE(dev_priv, _VCS(i)))
>>                         continue;
>>
>> -               if (!(BIT(i) & RUNTIME_INFO(dev_priv)->vdbox_enable)) {
>> +               if (!(BIT(i) & vdbox_mask)) {
>>                         info->engine_mask &= ~BIT(_VCS(i));
>>                         DRM_DEBUG_DRIVER("vcs%u fused off\n", i);
>>                         continue;
>> @@ -900,12 +901,12 @@ void intel_device_info_init_mmio(struct  
>> drm_i915_private *dev_priv)
>>                         RUNTIME_INFO(dev_priv)->vdbox_sfc_access |=  
>> BIT(i);
>>         }
>>
>> -       DRM_DEBUG_DRIVER("vebox enable: %04x\n",  
>> RUNTIME_INFO(dev_priv)->vebox_enable);
>> +       DRM_DEBUG_DRIVER("vebox enable: %04x\n", vebox_mask);
>>         for (i = 0; i < I915_MAX_VECS; i++) {
>>                 if (!HAS_ENGINE(dev_priv, _VECS(i)))
>>                         continue;
>>
>> -               if (!(BIT(i) & RUNTIME_INFO(dev_priv)->vebox_enable)) {
>> +               if (!(BIT(i) & vebox_mask)) {
>>                         info->engine_mask &= ~BIT(_VECS(i));
>>                         DRM_DEBUG_DRIVER("vecs%u fused off\n", i);
>>                 }
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h  
>> b/drivers/gpu/drm/i915/intel_device_info.h
>> index 6234570a9b17..06f428c70ff5 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -208,10 +208,6 @@ struct intel_runtime_info {
>>
>>         u32 cs_timestamp_frequency_khz;
>>
>> -       /* Enabled (not fused off) media engine bitmasks. */
>> -       u8 vdbox_enable;
>> -       u8 vebox_enable;
>> -
>>         /* Media engine access to SFC per instance */
>>         u8 vdbox_sfc_access;
>
> Code looks ok, just checkpatch nits
>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>
> Michal, as the most likely user, what do you think?
> -Chris

Can't spot any other nits,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>

~Michal