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

[Thomas Zimmermann]

Hi

Am 20.06.22 um 16:45 schrieb Thomas Zimmermann:
> 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.

I guess that if you want to change something here, you could add 
drmm_encoder_init() instead and convert vc4. That function might be more 
useful for other drivers in the long run.

Best regards
Thomas

> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220620/f3a21f80/attachment.sig>