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