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

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

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.

-- 
Best regards,
Dmitry