[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

Hi

Am 23.07.20 um 16:46 schrieb Philipp Zabel:
> Hi Thomas,
> 
> On Thu, 2020-07-23 at 09:35 +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> I have meanwhile seen your imx patchset where this would be useful.
>>
>> I still think you should try to pre-allocated all encoders up to a
>> limit, so that an extra drmm_kzalloc() is not required. But see my
>> comments below.
> 
> Thank you for the review coments. The complication with imx-drm is that
> the encoders are all in separate platform devices, using the component
> framework. Preallocating encoders in the main driver would be
> impractical.
> 
> The encoders are added in the component .bind() callback, so the main
> driver must call drmm_mode_config_init() before binding all components.
> The bind callback also is the first place where the component drivers
> get to know the drm device, so it is not possible to use drmm_kzalloc()
> any earlier.
> 
>> Am 22.07.20 um 15:25 schrieb Philipp Zabel:
>>> Add a drm_simple_encoder_init() variant that registers
>>> drm_encoder_cleanup() with drmm_add_action().
>>>
>>> Now drivers can store encoders in memory allocated with drmm_kmalloc()
>>> after the call to drmm_mode_config_init(), without having to manually
>>> make sure that drm_encoder_cleanup() is called before the memory is
>>> freed.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
>>> ---
>>>  drivers/gpu/drm/drm_simple_kms_helper.c | 42 +++++++++++++++++++++++++
>>>  include/drm/drm_simple_kms_helper.h     |  4 +++
>>>  2 files changed, 46 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> index 74946690aba4..a243f00cf63d 100644
>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> @@ -9,6 +9,7 @@
>>>  #include <drm/drm_atomic.h>
>>>  #include <drm/drm_atomic_helper.h>
>>>  #include <drm/drm_bridge.h>
>>> +#include <drm/drm_managed.h>
>>>  #include <drm/drm_plane_helper.h>
>>>  #include <drm/drm_probe_helper.h>
>>>  #include <drm/drm_simple_kms_helper.h>
>>> @@ -71,6 +72,47 @@ int drm_simple_encoder_init(struct drm_device *dev,
>>>  }
>>>  EXPORT_SYMBOL(drm_simple_encoder_init);
>>>  
>>> +static void drmm_encoder_cleanup(struct drm_device *dev, void *ptr)
>>
>> It's the reset helper, so drmm_simple_encoder_reset() would be appropriate.
>>
>>> +{
>>> +	struct drm_encoder *encoder = ptr;
>>> +
>>> +	drm_encoder_cleanup(encoder);
>>
>> This should first check for (encoder->dev) being true. If drivers
>> somehow manage to clean-up the mode config first, we should detect it. I
>> know it's a bug, but I wouldn't trust drivers with that.
> 
> I don't think this can happen, a previously called drm_encoder_cleanup()
> would have removed the encoder from the drm_mode_config::encoder list.

It cannot legally happen, but AFAICS a careless driver could run
drm_mode_config_cleanup() and release the encoder. This release callback
would still run afterwards and it should warn about the error.

> 
>>> +}
>>> +
>>> +/**
>>> + * drmm_simple_encoder_init - Initialize a preallocated encoder with
>>> + *                            basic functionality.
>>> + * @dev: drm device
>>> + * @encoder: the encoder to initialize
>>> + * @encoder_type: user visible type of the encoder
>>> + *
>>> + * Initialises a preallocated encoder that has no further functionality.
>>
>> 'Initializes'
> 
> Copy & paste from the drm_simple_encoder_init, I'll fix this in the next
> version.

Yeah. That was originally my typo. :p

> 
>>> + * Settings for possible CRTC and clones are left to their initial values.
>>> + * Cleanup is automatically handled through registering drm_encoder_cleanup()
>>> + * with drmm_add_action().
>>> + *
>>> + * The caller of drmm_simple_encoder_init() is responsible for allocating
>>> + * the encoder's memory with drmm_kzalloc() to ensure it is automatically
>>> + * freed after the encoder has been cleaned up.
>>> + *
>>
>> The idiomatic way of cleaning up an encoder is via mode-config cleanup.
>> This interface is an exception for a corner case. So there needs to be a
>> paragraph that clearly explains the corner case. Please also discourage
>> from using drmm_simple_encoder_init() if drm_simple_encoder_init() would
>> work.
> 
> I was hoping that we would eventually switch to drmres cleanup as the
> preferred method, thus getting rid of the need for per-driver cleanup in
> the error paths and destroy callbacks in most cases.

True. I do support that. But we should also consider how to do this
efficiently. Using drmm_mode_config_init() sets up a release function
that handles all initialized encoders. For the majority of drivers, this
is sufficient. Using drmm_encoder_init() in those drivers just adds
overhead without additional benefits. That's also why I consider imx'
requirements a corner case.

Best regards
Thomas

> 
> regards
> Philipp
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200724/01ffe30c/attachment-0001.sig>