[PATCH v2 2/2] tools/gputop: toggle sysfs profiling knob if available for device

[Tvrtko Ursulin]

On 15/04/2024 20:31, Adrián Larumbe wrote:
> On 03.04.2024 15:46, Lucas De Marchi wrote:
>> On Tue, Apr 02, 2024 at 11:27:45PM +0100, Adrián Larumbe wrote:
>>> For every DRM device that enables its job accounting HW from user space,
>>> toggle it right before obtaining per-client fdinfo numbers.
>>>
>>> Make sure profiling status is returned to its original state before
>>> exiting, by handling the SIGINT signal just like in intel_gpu_top.
>>
>> isn't this something that should be rather tracked on the kernel side
>> rather than relying on the userspace to behave?
> 
> This would simplify handling of the sysfs knob state from user space, but I'm
> not sure normal sysfs device attributes are allowed to keep an internal state
> between _show() calls.
> 
>> 1) call gputop
>> 2) call gputop, because you forgot you had it open in another ssh
>>     session
>> 3) stop (1), because ssh session dies
> 
> I feel tempted to brush this off as legitimate behaviour, albeit 'undefined'.
> If we cannot handle the scenario in which multiple profilers tweak the profiling
> state at the same time from kernel space, then I would prefer to expect users to
> readjust the sysfs attribute value manually.

I agree with Lucas that would be a pretty bad user experience. We have 
to consider if more users for fdinfo appear there could be more fighting 
over this control going on. And I don't see any reasonable solution 
within the sysfs framework. It is not possible to have own fops in 
there, is it?

Would it be of any use if gputop would be checking the state of the knob 
from the main loop so it can at least give explicit notification someone 
toggled it to off? (Instead of suddenly and silently showing no GPU 
utilisation.) Maybe also before trying to restore it. It would be racy 
yes but maybe good enough for practical purposes?

Regards,

Tvrtko

> 
>>  From the kernel side, I think it could be done by enabling it on fd
>> open, disabling it on last close.
> 
> Adrian Larumbe