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

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

[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);
 	drm_encoder_helper_add(encoder, &virtio_gpu_enc_helper_funcs);
 	encoder->possible_crtcs = 1 << index;
 
-- 
2.53.0

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

[Thomas Zimmermann]

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

Best regards
Thomas

>   	drm_encoder_helper_add(encoder, &virtio_gpu_enc_helper_funcs);
>   	encoder->possible_crtcs = 1 << index;
>   

-- 
--
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)

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

[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?

-- 
Best regards,
Dmitry

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

[Thomas Zimmermann]

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.

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)

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

[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.

-- 
Best regards,
Dmitry

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

[Thomas Zimmermann]

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)

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

[Dmitry Osipenko]

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