Re: [PATCH] drm/virtio: Open-code drm_simple_encoder_init()

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 3/2/26 18:40, Thomas Zimmermann wrote:
> 
> 
> Am 02.03.26 um 16:31 schrieb Dmitry Osipenko:
>> On 3/2/26 17:15, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 02.03.26 um 14:57 schrieb Dmitry Osipenko:
>>>> On 3/2/26 16:48, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 27.02.26 um 11:35 schrieb Hardik Phalet:
>>>>>> drm_simple_encoder_init() is a thin wrapper around drm_encoder_init()
>>>>>> that only provides a minimal drm_encoder_funcs instance with
>>>>>> .destroy = drm_encoder_cleanup.
>>>>>>
>>>>>> Inline the helper in virtgpu_display.c and provide a local
>>>>>> drm_encoder_funcs instance instead. This removes the unnecessary
>>>>>> indirection and prepares for the eventual removal of
>>>>>> drm_simple_encoder_init().
>>>>>>
>>>>>> No functional changes intended.
>>>>>>
>>>>>> Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>
>>>>>> ---
>>>>>>     drivers/gpu/drm/virtio/virtgpu_display.c | 7 ++++++-
>>>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/
>>>>>> drm/virtio/virtgpu_display.c
>>>>>> index f1dae9569805..8bd6cdc6c16e 100644
>>>>>> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
>>>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
>>>>>> @@ -232,6 +232,10 @@ static enum drm_mode_status
>>>>>> virtio_gpu_conn_mode_valid(struct drm_connector *con
>>>>>>         return MODE_BAD;
>>>>>>     }
>>>>>>     +static const struct drm_encoder_funcs
>>>>>> virtio_gpu_enc_cleanup_funcs = {
>>>>>> +    .destroy = drm_encoder_cleanup
>>>>>> +};
>>>>>> +
>>>>>>     static const struct drm_encoder_helper_funcs
>>>>>> virtio_gpu_enc_helper_funcs = {
>>>>>>         .mode_set   = virtio_gpu_enc_mode_set,
>>>>>>         .enable     = virtio_gpu_enc_enable,
>>>>>> @@ -306,7 +310,8 @@ static int vgdev_output_init(struct
>>>>>> virtio_gpu_device *vgdev, int index)
>>>>>>         if (vgdev->has_edid)
>>>>>>             drm_connector_attach_edid_property(connector);
>>>>>>     -    drm_simple_encoder_init(dev, encoder,
>>>>>> DRM_MODE_ENCODER_VIRTUAL);
>>>>>> +    drm_encoder_init(dev, encoder, &virtio_gpu_enc_cleanup_funcs,
>>>>>> +             DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>> This looks correct, but you should also remove the include statement
>>>>> at [1]
>>>>>
>>>>> [1] https://elixir.bootlin.com/linux/v6.19/source/drivers/gpu/drm/
>>>>> virtio/virtgpu_display.c#L35
>>>> The patch adds more lines than removes. What's wrong with
>>>> drm_simple_encoder_init() and why it needs to be removed eventually?
>>> I added it myself a few years ago in an attempt to save some lines of
>>> code. That was a mistake. It's a helper without any purpose. Helpers
>>> should do something.
>> It saves few lines and makes code easier to read. Don't see value in
>> removal of the helper.
> 
> All it does is to set a default cleanup function. But that's not even
> clear from the helper's name.
> 
> If we really want a default cleanup, we should call
> drm_encoder_cleanup() as a default at [1]. Drivers could then just leave
> out the encoder funcs entirely.
> 
> [1] https://elixir.bootlin.com/linux/v6.19/source/drivers/gpu/drm/
> drm_mode_config.c#L530

There are a lot of drivers using drm_simple_encoder_init() in kernel.
What problem the default cleanup function creates? If none, to me the
drm_simple_encoder_init() serves its purpose, improving the drivers'
code. I won't object much if you'll insist on merging the patch, but so
far it looks like a very bikeshed change without a clear explanation of
the solved problem.

-- 
Best regards,
Dmitry