DRM accel and debugfs/sysfs

DRM accel and debugfs/sysfs

[Stanislaw Gruszka]

Hi

I was about to send debugfs support for ivpu and noticed that there
are current changes that deprecate drm_devel->debugfs_init callback.

Further I looked at this commit [1], that stated we should not
use drm_minor for debugfs and sysfs. What is quite contrary to 
what drm accel framework did in the first place. 

So my question is how we should use debugfs/sysfs in accel?
Use it with old fashioned minor-centric way or change
the framework somehow ?

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26

Regards
Stanislaw

Re: DRM accel and debugfs/sysfs

[Maíra Canal]

Hi Stanislaw,

On 2/1/23 12:20, Stanislaw Gruszka wrote:
> Hi
> 
> I was about to send debugfs support for ivpu and noticed that there
> are current changes that deprecate drm_devel->debugfs_init callback.
> 
> Further I looked at this commit [1], that stated we should not
> use drm_minor for debugfs and sysfs. What is quite contrary to
> what drm accel framework did in the first place.
> 
> So my question is how we should use debugfs/sysfs in accel?
> Use it with old fashioned minor-centric way or change
> the framework somehow ?

As we are trying to replace drm_debugfs_create_files() [1], it would
be nice to see the accel debugfs support use the new debugfs API. This
would mean using the debugfs_list from the drm_device, deprecating
the debugfs_init callback, and adding the a similar code snippet to
accel_debugfs_init:

list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
	debugfs_create_file(entry->file.name, 0444,
			    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
	list_del(&entry->list);

Maybe Daniel has some more thoughts on this matter, but I guess it
would be better to drop the use of the old-fashioned minor-centric
implementation in accel.

[1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511

Best Regards,
- Maíra Canal

> 
> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
> 
> Regards
> Stanislaw
> 

Re: DRM accel and debugfs/sysfs

[Jeffrey Hugo]

On 2/7/2023 4:31 AM, Maíra Canal wrote:
> Hi Stanislaw,
> 
> On 2/1/23 12:20, Stanislaw Gruszka wrote:
>> Hi
>>
>> I was about to send debugfs support for ivpu and noticed that there
>> are current changes that deprecate drm_devel->debugfs_init callback.
>>
>> Further I looked at this commit [1], that stated we should not
>> use drm_minor for debugfs and sysfs. What is quite contrary to
>> what drm accel framework did in the first place.
>>
>> So my question is how we should use debugfs/sysfs in accel?
>> Use it with old fashioned minor-centric way or change
>> the framework somehow ?
> 
> As we are trying to replace drm_debugfs_create_files() [1], it would
> be nice to see the accel debugfs support use the new debugfs API. This
> would mean using the debugfs_list from the drm_device, deprecating
> the debugfs_init callback, and adding the a similar code snippet to
> accel_debugfs_init:
> 
> list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
>      debugfs_create_file(entry->file.name, 0444,
>                  minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>      list_del(&entry->list);
> 
> Maybe Daniel has some more thoughts on this matter, but I guess it
> would be better to drop the use of the old-fashioned minor-centric
> implementation in accel.
> 
> [1] 
> https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511 
> 
> 
> Best Regards,
> - Maíra Canal

Thank you for the details Maira.  It helps to explain what the todo is 
suggesting.  Is there an example of a driver/drm_device that uses 
debugfs_list which you can easily point to?

-Jeff

> 
>>
>> [1] 
>> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26 
>>
>>
>> Regards
>> Stanislaw
>>

Re: DRM accel and debugfs/sysfs

[Maíra Canal]

On 2/7/23 12:43, Jeffrey Hugo wrote:
> On 2/7/2023 4:31 AM, Maíra Canal wrote:
>> Hi Stanislaw,
>>
>> On 2/1/23 12:20, Stanislaw Gruszka wrote:
>>> Hi
>>>
>>> I was about to send debugfs support for ivpu and noticed that there
>>> are current changes that deprecate drm_devel->debugfs_init callback.
>>>
>>> Further I looked at this commit [1], that stated we should not
>>> use drm_minor for debugfs and sysfs. What is quite contrary to
>>> what drm accel framework did in the first place.
>>>
>>> So my question is how we should use debugfs/sysfs in accel?
>>> Use it with old fashioned minor-centric way or change
>>> the framework somehow ?
>>
>> As we are trying to replace drm_debugfs_create_files() [1], it would
>> be nice to see the accel debugfs support use the new debugfs API. This
>> would mean using the debugfs_list from the drm_device, deprecating
>> the debugfs_init callback, and adding the a similar code snippet to
>> accel_debugfs_init:
>>
>> list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
>>      debugfs_create_file(entry->file.name, 0444,
>>                  minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>>      list_del(&entry->list);
>>
>> Maybe Daniel has some more thoughts on this matter, but I guess it
>> would be better to drop the use of the old-fashioned minor-centric
>> implementation in accel.
>>
>> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
>>
>> Best Regards,
>> - Maíra Canal
> 
> Thank you for the details Maira.  It helps to explain what the todo is suggesting.  Is there an example of a driver/drm_device that uses debugfs_list which you can easily point to?

The implementation of this device-centered infrastructure is linked in [1]
and an example of the conversion of debugfs APIs is linked in [2], and other
drivers such as v3d, vkms, vc4 and gud use this new API as well.

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653
[2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2e3ab8a6994f265bbd4dbd00448b84548f18464c

Best Regards,
- Maíra Canal

> 
> -Jeff
> 
>>
>>>
>>> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
>>>
>>> Regards
>>> Stanislaw
>>>
> 

Re: DRM accel and debugfs/sysfs

[Daniel Vetter]

On Tue, Feb 07, 2023 at 01:17:47PM -0300, Maíra Canal wrote:
> On 2/7/23 12:43, Jeffrey Hugo wrote:
> > On 2/7/2023 4:31 AM, Maíra Canal wrote:
> > > Hi Stanislaw,
> > > 
> > > On 2/1/23 12:20, Stanislaw Gruszka wrote:
> > > > Hi
> > > > 
> > > > I was about to send debugfs support for ivpu and noticed that there
> > > > are current changes that deprecate drm_devel->debugfs_init callback.
> > > > 
> > > > Further I looked at this commit [1], that stated we should not
> > > > use drm_minor for debugfs and sysfs. What is quite contrary to
> > > > what drm accel framework did in the first place.
> > > > 
> > > > So my question is how we should use debugfs/sysfs in accel?
> > > > Use it with old fashioned minor-centric way or change
> > > > the framework somehow ?
> > > 
> > > As we are trying to replace drm_debugfs_create_files() [1], it would
> > > be nice to see the accel debugfs support use the new debugfs API. This
> > > would mean using the debugfs_list from the drm_device, deprecating
> > > the debugfs_init callback, and adding the a similar code snippet to
> > > accel_debugfs_init:
> > > 
> > > list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> > >      debugfs_create_file(entry->file.name, 0444,
> > >                  minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> > >      list_del(&entry->list);
> > > 
> > > Maybe Daniel has some more thoughts on this matter, but I guess it
> > > would be better to drop the use of the old-fashioned minor-centric
> > > implementation in accel.

It was a simple case of two things landing in parallel and not being
synchronized. Would be good if accel could be adapted to use the new
debugfs infra, now that both accel and the new debugfs stuff have landed.
-Daniel

> > > 
> > > [1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
> > > 
> > > Best Regards,
> > > - Maíra Canal
> > 
> > Thank you for the details Maira.  It helps to explain what the todo is suggesting.  Is there an example of a driver/drm_device that uses debugfs_list which you can easily point to?
> 
> The implementation of this device-centered infrastructure is linked in [1]
> and an example of the conversion of debugfs APIs is linked in [2], and other
> drivers such as v3d, vkms, vc4 and gud use this new API as well.
> 
> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653
> [2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2e3ab8a6994f265bbd4dbd00448b84548f18464c
> 
> Best Regards,
> - Maíra Canal
> 
> > 
> > -Jeff
> > 
> > > 
> > > > 
> > > > [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
> > > > 
> > > > Regards
> > > > Stanislaw
> > > > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: DRM accel and debugfs/sysfs

[Oded Gabbay]

On Wed, Feb 8, 2023 at 8:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Feb 07, 2023 at 01:17:47PM -0300, Maíra Canal wrote:
> > On 2/7/23 12:43, Jeffrey Hugo wrote:
> > > On 2/7/2023 4:31 AM, Maíra Canal wrote:
> > > > Hi Stanislaw,
> > > >
> > > > On 2/1/23 12:20, Stanislaw Gruszka wrote:
> > > > > Hi
> > > > >
> > > > > I was about to send debugfs support for ivpu and noticed that there
> > > > > are current changes that deprecate drm_devel->debugfs_init callback.
> > > > >
> > > > > Further I looked at this commit [1], that stated we should not
> > > > > use drm_minor for debugfs and sysfs. What is quite contrary to
> > > > > what drm accel framework did in the first place.
> > > > >
> > > > > So my question is how we should use debugfs/sysfs in accel?
> > > > > Use it with old fashioned minor-centric way or change
> > > > > the framework somehow ?
> > > >
> > > > As we are trying to replace drm_debugfs_create_files() [1], it would
> > > > be nice to see the accel debugfs support use the new debugfs API. This
> > > > would mean using the debugfs_list from the drm_device, deprecating
> > > > the debugfs_init callback, and adding the a similar code snippet to
> > > > accel_debugfs_init:
> > > >
> > > > list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> > > >      debugfs_create_file(entry->file.name, 0444,
> > > >                  minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> > > >      list_del(&entry->list);
> > > >
> > > > Maybe Daniel has some more thoughts on this matter, but I guess it
> > > > would be better to drop the use of the old-fashioned minor-centric
> > > > implementation in accel.
>
> It was a simple case of two things landing in parallel and not being
> synchronized. Would be good if accel could be adapted to use the new
> debugfs infra, now that both accel and the new debugfs stuff have landed.
> -Daniel
Yes, definitely.
Does anyone volunteer to send a patch to align ?
If not, we will do it internally and send a patch.

Oded
>
> > > >
> > > > [1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
> > > >
> > > > Best Regards,
> > > > - Maíra Canal
> > >
> > > Thank you for the details Maira.  It helps to explain what the todo is suggesting.  Is there an example of a driver/drm_device that uses debugfs_list which you can easily point to?
> >
> > The implementation of this device-centered infrastructure is linked in [1]
> > and an example of the conversion of debugfs APIs is linked in [2], and other
> > drivers such as v3d, vkms, vc4 and gud use this new API as well.
> >
> > [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653
> > [2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2e3ab8a6994f265bbd4dbd00448b84548f18464c
> >
> > Best Regards,
> > - Maíra Canal
> >
> > >
> > > -Jeff
> > >
> > > >
> > > > >
> > > > > [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
> > > > >
> > > > > Regards
> > > > > Stanislaw
> > > > >
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: DRM accel and debugfs/sysfs

[Maíra Canal]

On 2/8/23 15:13, Oded Gabbay wrote:
> On Wed, Feb 8, 2023 at 8:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Tue, Feb 07, 2023 at 01:17:47PM -0300, Maíra Canal wrote:
>>> On 2/7/23 12:43, Jeffrey Hugo wrote:
>>>> On 2/7/2023 4:31 AM, Maíra Canal wrote:
>>>>> Hi Stanislaw,
>>>>>
>>>>> On 2/1/23 12:20, Stanislaw Gruszka wrote:
>>>>>> Hi
>>>>>>
>>>>>> I was about to send debugfs support for ivpu and noticed that there
>>>>>> are current changes that deprecate drm_devel->debugfs_init callback.
>>>>>>
>>>>>> Further I looked at this commit [1], that stated we should not
>>>>>> use drm_minor for debugfs and sysfs. What is quite contrary to
>>>>>> what drm accel framework did in the first place.
>>>>>>
>>>>>> So my question is how we should use debugfs/sysfs in accel?
>>>>>> Use it with old fashioned minor-centric way or change
>>>>>> the framework somehow ?
>>>>>
>>>>> As we are trying to replace drm_debugfs_create_files() [1], it would
>>>>> be nice to see the accel debugfs support use the new debugfs API. This
>>>>> would mean using the debugfs_list from the drm_device, deprecating
>>>>> the debugfs_init callback, and adding the a similar code snippet to
>>>>> accel_debugfs_init:
>>>>>
>>>>> list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
>>>>>       debugfs_create_file(entry->file.name, 0444,
>>>>>                   minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>>>>>       list_del(&entry->list);
>>>>>
>>>>> Maybe Daniel has some more thoughts on this matter, but I guess it
>>>>> would be better to drop the use of the old-fashioned minor-centric
>>>>> implementation in accel.
>>
>> It was a simple case of two things landing in parallel and not being
>> synchronized. Would be good if accel could be adapted to use the new
>> debugfs infra, now that both accel and the new debugfs stuff have landed.
>> -Daniel
> Yes, definitely.
> Does anyone volunteer to send a patch to align ?

I can send a patch this week align it.

Best Regards,
- Maíra Canal

> If not, we will do it internally and send a patch.
> 
> Oded
>>
>>>>>
>>>>> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
>>>>>
>>>>> Best Regards,
>>>>> - Maíra Canal
>>>>
>>>> Thank you for the details Maira.  It helps to explain what the todo is suggesting.  Is there an example of a driver/drm_device that uses debugfs_list which you can easily point to?
>>>
>>> The implementation of this device-centered infrastructure is linked in [1]
>>> and an example of the conversion of debugfs APIs is linked in [2], and other
>>> drivers such as v3d, vkms, vc4 and gud use this new API as well.
>>>
>>> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653
>>> [2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2e3ab8a6994f265bbd4dbd00448b84548f18464c
>>>
>>> Best Regards,
>>> - Maíra Canal
>>>
>>>>
>>>> -Jeff
>>>>
>>>>>
>>>>>>
>>>>>> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
>>>>>>
>>>>>> Regards
>>>>>> Stanislaw
>>>>>>
>>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: DRM accel and debugfs/sysfs

[Maíra Canal]

On 2/8/23 15:13, Oded Gabbay wrote:
> On Wed, Feb 8, 2023 at 8:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Tue, Feb 07, 2023 at 01:17:47PM -0300, Maíra Canal wrote:
>>> On 2/7/23 12:43, Jeffrey Hugo wrote:
>>>> On 2/7/2023 4:31 AM, Maíra Canal wrote:
>>>>> Hi Stanislaw,
>>>>>
>>>>> On 2/1/23 12:20, Stanislaw Gruszka wrote:
>>>>>> Hi
>>>>>>
>>>>>> I was about to send debugfs support for ivpu and noticed that there
>>>>>> are current changes that deprecate drm_devel->debugfs_init callback.
>>>>>>
>>>>>> Further I looked at this commit [1], that stated we should not
>>>>>> use drm_minor for debugfs and sysfs. What is quite contrary to
>>>>>> what drm accel framework did in the first place.
>>>>>>
>>>>>> So my question is how we should use debugfs/sysfs in accel?
>>>>>> Use it with old fashioned minor-centric way or change
>>>>>> the framework somehow ?
>>>>>
>>>>> As we are trying to replace drm_debugfs_create_files() [1], it would
>>>>> be nice to see the accel debugfs support use the new debugfs API. This
>>>>> would mean using the debugfs_list from the drm_device, deprecating
>>>>> the debugfs_init callback, and adding the a similar code snippet to
>>>>> accel_debugfs_init:
>>>>>
>>>>> list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
>>>>>       debugfs_create_file(entry->file.name, 0444,
>>>>>                   minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>>>>>       list_del(&entry->list);
>>>>>
>>>>> Maybe Daniel has some more thoughts on this matter, but I guess it
>>>>> would be better to drop the use of the old-fashioned minor-centric
>>>>> implementation in accel.
>>
>> It was a simple case of two things landing in parallel and not being
>> synchronized. Would be good if accel could be adapted to use the new
>> debugfs infra, now that both accel and the new debugfs stuff have landed.
>> -Daniel
> Yes, definitely.
> Does anyone volunteer to send a patch to align ?
> If not, we will do it internally and send a patch.

Christian sent today a patchset addressing some problems in the debugfs
API [1]. As he is planning to remove the debugfs_list, I guess it would
be better to wait a while to align accel with the debugfs API,
considering that it currently changing.

Also, it would be nice to get feedback from the accel side.

[1] https://lore.kernel.org/dri-devel/613b9aec-7105-ca2d-13cd-16ddd85a6fda@igalia.com/T/

Best Regards,
- Maíra Canal

> 
> Oded
>>
>>>>>
>>>>> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
>>>>>
>>>>> Best Regards,
>>>>> - Maíra Canal
>>>>
>>>> Thank you for the details Maira.  It helps to explain what the todo is suggesting.  Is there an example of a driver/drm_device that uses debugfs_list which you can easily point to?
>>>
>>> The implementation of this device-centered infrastructure is linked in [1]
>>> and an example of the conversion of debugfs APIs is linked in [2], and other
>>> drivers such as v3d, vkms, vc4 and gud use this new API as well.
>>>
>>> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653
>>> [2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2e3ab8a6994f265bbd4dbd00448b84548f18464c
>>>
>>> Best Regards,
>>> - Maíra Canal
>>>
>>>>
>>>> -Jeff
>>>>
>>>>>
>>>>>>
>>>>>> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a26
>>>>>>
>>>>>> Regards
>>>>>> Stanislaw
>>>>>>
>>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: DRM accel and debugfs/sysfs

[Stanislaw Gruszka]

On Thu, Feb 09, 2023 at 10:17:26AM -0300, Maíra Canal wrote:
> Christian sent today a patchset addressing some problems in the debugfs
> API [1]. As he is planning to remove the debugfs_list, I guess it would
> be better to wait a while to align accel with the debugfs API,
> considering that it currently changing.
> 
> Also, it would be nice to get feedback from the accel side.
> 
> [1] https://lore.kernel.org/dri-devel/613b9aec-7105-ca2d-13cd-16ddd85a6fda@igalia.com/T/

I'll cc accel people and continue the discussion there.

Regards
Stanislaw