Hi

Am 22.07.20 um 17:08 schrieb Philipp Zabel:
> Hi Thomas,
> 
> thank you for your comment.
> 
> On Wed, 2020-07-22 at 16:43 +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> 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@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)
>>> +{
>>> +	struct drm_encoder *encoder = ptr;
>>> +
>>> +	drm_encoder_cleanup(encoder);
>>> +}
>>
>> This doesn't work. DRM cleans up the encoder by invoking the destroy
>> callback from the encoder functions. This additional helper would
>> cleanup the encoder a second time.
> 
> Indeed this would require the encoder destroy callback to be NULL.
> 
>> You can already embed the encoder in another structure and things should
>> work as expected.
> 
> If the embedding structure is a component allocated with drmm_kmalloc()
> after the call to drmm_mode_config_init(), the structure will already be
> freed before the destroy callback is run from
> drmm_mode_config_init_release().

Why not put the kamlloc before the mode_config_init? Is there some
complicated dependency?

As the number of encoders is limited, one could also embed the maximum
number of instances.

The purpose of simple encoder is to move the function structure to a
single place. IMHO if you need something special, such as in the given
drmm_kmalloc() example, you should roll your own in the driver.

Best regards
Thomas

> 
> regards
> Philipp
> _______________________________________________
> dri-devel mailing list
> dri-devel@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