[PATCH 2/2] drm/amdgpu: Use mod_delayed_work in JPEG/UVD/VCE/VCN ring_end_use hooks

[Lazar, Lijo]

On 8/12/2021 1:41 PM, Michel Dänzer wrote:
> On 2021-08-12 7:55 a.m., Koenig, Christian wrote:
>> Hi James,
>>
>> Evan seems to have understood how this all works together.
>>
>> See while any begin/end use critical section is active the work should not be active.
>>
>> When you handle only one ring you can just call cancel in begin use and schedule in end use. But when you have more than one ring you need a lock or counter to prevent concurrent work items to be started.
>>
>> Michelle's idea to use mod_delayed_work is a bad one because it assumes that the delayed work is still running.
> 
> It merely assumes that the work may already have been scheduled before.
> 
> Admittedly, I missed the cancel_delayed_work_sync calls for patch 2. While I think it can still have some effect when there's a single work item for multiple rings, as described by James, it's probably negligible, since presumably the time intervals between ring_begin_use and ring_end_use are normally much shorter than a second.
> 
> So, while patch 2 is at worst a no-op (since mod_delayed_work is the same as schedule_delayed_work if the work hasn't been scheduled yet), I'm fine with dropping it.
> 
> 
>> Something similar applies to the first patch I think,
> 
> There are no cancel work calls in that case, so the commit log is accurate TTBOMK.

Curious -

For patch 1, does it make a difference if any delayed work scheduled is 
cancelled in the else part before proceeding?

} else if (!enable && adev->gfx.gfx_off_state) {
cancel_delayed_work();


Thanks,
Lijo

> 
> I noticed this because current mutter Git main wasn't able to sustain 60 fps on Navi 14 with a simple glxgears -fullscreen. mutter was dropping frames because its CPU work for a frame update occasionally took up to 3 ms, instead of the normal 2-300 microseconds. sysprof showed a lot of cycles spent in the functions which enable/disable GFXOFF in the HW.
> 
> 
>> so when this makes a difference it is actually a bug.
> 
> There was certainly a bug though, which patch 1 fixes. :)
> 
>