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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)