[Intel-gfx] [PATCH 2/2] drm/i915: Test all fw tables during mock selftests

[Tvrtko Ursulin]

On 09/12/2016 10:18, Chris Wilson wrote:
> On Fri, Dec 09, 2016 at 09:41:30AM +0000, Tvrtko Ursulin wrote:
>>
>> On 08/12/2016 22:29, Chris Wilson wrote:
>>> On Thu, Dec 08, 2016 at 04:52:24PM +0000, Tvrtko Ursulin wrote:
>>>> Idea for another late test:
>>>>
>>>> for (offset = 0; offset < 0x40000; offset++) {
>>>> 	fw_domain = intel_uncore_forcewake_for_reg(dev_priv, { .reg =
>>>> offset }, FW_REG_READ | FW_REG_WRITE);
>>>> 	if (WARN_ON(fw_domain & ~dev_priv->uncore.fw_domains))
>>>> 		return -EINVAL;
>>>> }
>>>>
>>>> And then we could convert the existing related WARNs in the live
>>>> code base to GEM_WARN_ONs.
>>>
>>> I liked it enough to type something up... However, isn't the argument
>>> for skipping some fw_domains that the associated register banks are
>>> invalid/absent on those platforms? i.e. we can't simply walk
>>> 	for (offset = 0; NEEDS_FORCE_WAKE(offset); offset++)
>>> and expect every offset to correspond to a register (and so need a
>>> covering fw_domain)?
>>
>> I thought it won't be a problem because it would return 0 in those
>> cases. So only those offsets that need a forcewake in a platform can
>> trigger the fail.
>>
>> Oh I see. We would need another layer before the condition checking, like:
>>
>> fw_domains = intel_uncore_forcewake_for_reg(...)
>> if ((fw_domains & FORCEWAKE_MEDIA) && !HAS_ENGINE(VCS))
>> 	fw_domains &= ~FORCEWAKE_MEDIA;
>
> That's almost like saying we should just
> 	return entry->domain & i915->uncore.fw_domains;
> which we do anyway by the filtering in force_wake_auto.

Haha yes, so my proposed test is completely pointless. :)

All forcewake get functions mask with i915->uncore.fw_domains so there 
is no potential to access an uninitialized domain.

WARN_ON(fw_domain & ~dev_priv->uncore.fw_domains) elsewhere in the code 
have to be really removed for future proofing it seems to me.

> Hmm. Could we use the mmio debug. Something like
>
> 	valid_reg = bitmap_create(0x40000)
> 	FORCEWAKE_ALL
> 	for_each_offset()
> 		read reg
> 		if (!mmio_debug)
> 			set_bit(valid_reg);
>
> 	for_each_bit()
> 		FORCEWAKE_DISABLE
> 		read reg using fw_domains
> 		if (mmio_debug)
> 			AWOOGA!
> ?
>
> Hopefully read reg won't cause system hangs, and less liable to corrupt
> state than write reg.

I think we need to restate what are we actually trying to test. I at 
least am confused now. :)

Regards,

Tvrtko