On Thu, Dec 26, 2019 at 03:05:41PM -0800, Matt Roper wrote:
On Thu, Dec 26, 2019 at 02:23:49PM -0800, Lucas De Marchi wrote:
On Tue, Dec 24, 2019 at 03:15:21PM -0800, Matt Roper wrote:
> Our usual i915 convention is to assume that future platforms will follow
> the same behavior as the latest platform of today. The VDBOX/SFC
> capabilities described here don't seem like something that should be
> specific to TGL, so let's future-proof by making the test apply to all
> gen12+ platforms.
>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_device_info.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 1acb5db77431..bb709a08bd3c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -1093,7 +1093,7 @@ void intel_device_info_init_mmio(struct drm_i915_private *dev_priv)
> * hooked up to an SFC (Scaler & Format Converter) unit.
> * In TGL each VDBOX has access to an SFC.
> */
> - if (IS_TIGERLAKE(dev_priv) || logical_vdbox++ % 2 == 0)
> + if (INTEL_GEN(dev_priv) >= 12 || logical_vdbox++ % 2 == 0)
> RUNTIME_INFO(dev_priv)->vdbox_sfc_access |= BIT(i);
but why are we even doing this instead of initiliazing them at compile
time on the device_info? If it's fused off, then whatever is set in
vdbox_sfc_access bit shouldn't matter... or if the code making use of
this doesn't check for engine availability, then this part of the
function could just disable the bit of whatever is fused off, regardless
if it's ice lake, tiger lake or whatever.
I'm not sure; it looks like among other things we send this bitmask
directly to the GuC, so I'm not really comfortable making the assumption
that all users of the mask will pay attention to whether the engine is
fused off or not, even if that turns out to be true for the i915 usage.
I could switch this to being initialized statically and then modified
here if an engine is fused off. In that case should this move out of
RUNTIME_INFO() and back to INTEL_INFO()? Honestly I've never really
understood why we have those separated given that we still ignore the
const and modify INTEL_INFO at runtime in several places.
hummn... true, there's this problem of not being able to initialize it
statically in one place and possibly modify it later.
We can't move it to INTEL_INFO. Anything that can be modified in runtime
needs to be inside the runtime_info, not info. I think the goal with the
separation was to make it explicit what are the fields that can be
changed in runtime. We really can't change `info` since its static const
that can be shared (think binding the driver to more than one device).
It didn't matter much in the past since we were limited to 1.
And I think the reason to have the runtime_info const and discard it here
is because it should be in fact treated as const in all places except on
initialization: we don't want accidental changes of that struct.
So... I think there may be a better solution here, but I don't think
it's important enough to block this: it's always better to be consistent
to be able to refactor this when the needs arrive.
Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Lucas De Marchi
Matt
Lucas De Marchi
> }
> DRM_DEBUG_DRIVER("vdbox enable: %04x, instances: %04lx\n",
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
