Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 20.06.22 um 16:39 schrieb Maxime Ripard:
On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote:
Hi

Am 20.06.22 um 15:48 schrieb Maxime Ripard:
Hi,

On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote:
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
The DRM-managed function to register an encoder is
drmm_simple_encoder_alloc() and its variants, which will allocate the
underlying structure and initialisation the encoder.

However, we might want to separate the structure creation and the encoder
initialisation, for example if the structure is shared across multiple DRM
entities, for example an encoder and a connector.

Let's create an helper to only initialise an encoder that would be passed
as an argument.


There's nothing wrong with this patch, but I have to admit that adding
drm_simple_encoder_init() et al was a mistake.

Why do you think it was a mistake?

The simple-encoder stuff is an interface that no one really needs. Compared
to open-coding the function, it's barely an improvement in LOCs, but nothing
else.

Back when I added drm_simple_encoder_init(), I wanted to simplify the many
drivers that initialized the encoder with a cleanup callback and nothing
else.  IIRC it was an improvement back then.  But now we already have a
related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro
back then, we could use the regular encoder helpers.


It would have been better to add an initializer macro like

#define DRM_STATIC_ENCODER_DEFAULT_FUNCS \
    .destroy = drm_encoder_cleanup

It's way more flexible and similarly easy to use. Anyway...

We can still have this. It would simplify this series so I could
definitely squeeze that patch in and add a TODO item / deprecation
notice for simple encoders if you think it's needed

Not necessary. It's not super important.

The corollary is though :)

If I understand you right, it means that you'd rather have a destroy
callback everywhere instead of calling the _cleanup function through a
drm-managed callback, and let drm_dev_unregister do its job?

If so, it means that we shouldn't be following the drmm_.*_alloc
functions and should drop all the new ones from this series.

No, no. What I'm saying is that simple-encoder is a pointless mid-layer. There's nothing that couldn't easily be achieved with the regular encoder functions.

Best regards
Thomas


Maxime

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux