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