Re: [PATCH 6/9] drm/i915/execlists: Reset CSB write pointer after reset

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/06/2018 12:30, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-06-28 12:02:30)
>>
>> On 28/06/2018 11:17, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-06-28 11:06:25)
>>>>
>>>> On 27/06/2018 22:07, Chris Wilson wrote:
>>>>> On HW reset, the HW clears the write pointer (to 0). But since it also
>>>>> writes its first CSB entry to slot 0, we need to reset the write pointer
>>>>> back to the element before (so the first entry we read is 0).
>>>>>
>>>>> This is required for the next patch, where we trust the CSB completely!
>>>>>
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++--
>>>>>     1 file changed, 17 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> index 368a8c74d11d..8b111a268697 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> @@ -884,6 +884,21 @@ static void reset_irq(struct intel_engine_cs *engine)
>>>>>         clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>>>>>     }
>>>>>     
>>>>> +static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>>>>> +{
>>>>> +     /*
>>>>> +      * After a reset, the HW starts writing into CSB entry [0]. We
>>>>> +      * therefore have to set our HEAD pointer back one entry so that
>>>>> +      * the *first* entry we check is entry 0. To complicate this further,
>>>>> +      * as we don't wait for the first interrupt after reset, we have to
>>>>> +      * fake the HW write to point back to the last entry so that our
>>>>> +      * inline comparison of our cached head position against the last HW
>>>>> +      * write works even before the first interrupt.
>>>>> +      */
>>>>> +     execlists->csb_head = GEN8_CSB_ENTRIES - 1;
>>>>> +     WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16);
>>>>
>>>> Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK?
>>>
>>> Hah, there goes my attempt to kill off unused magic.
>>
>> At least _MASKED_FIELD makes it clearer.
>>
>> But the u8 trick is still evil since here you even explicitly do a fake
>> masked write on hwsp. Ugly and evil. How about storing
>> execlists->csb_write_default at init time and applying it here?
> 
> Honestly, I thought it made sense next to the READ_ONCE(*csb_write).
> 
> Could I just convince you with another comment pleading guilty to the
> atrocities?

I'd prefer we did not write random stuff into hwsp. Like if one day 
someone is debugging something, they could see either 0xff00?? or 0x?? 
in there depending on the timings. Then would have to waste time 
figuring out what's happening.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx