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 <[email protected]>
---
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)
