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. -- Best regards, Dmitry
