Hi
Am 23.07.20 um 11:05 schrieb Daniel Vetter:
> On Thu, Jul 23, 2020 at 9:36 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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.
>
> Uh preallocation is horribly, because you need to first preallocate
> all encoders, then call drmm_mode_config_init, and only then can you
> call drm_encoder_init. That's terrible flow, and I'm aware of the
Out of curiosity, what's the problem with the code flow? If you embed
everything in the device's structure, you'd pre-allocate automatically.
Then acquire one of the encoders just before drm_encoder_init(). Sure,
it needs a little helper to acquire and release the preallocated encoder
memory, but that's not that bad.
Best regards
Thomas
> problem. That's why we need new set of drmm_ helpers to do all the
> steps for kms static object setup, if we actually want to improve
> things.
> -Daniel
>
>>
>> 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@xxxxxxxxxxxxxx>
>>> ---
>>> 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.
>>
>>> +}
>>> +
>>> +/**
>>> + * 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'
>>
>>> + * 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.
>>
>> Best regards
>> Thomas
>>
>>> + * Returns:
>>> + * Zero on success, error code on failure.
>>> + */
>>> +int drmm_simple_encoder_init(struct drm_device *dev,
>>> + struct drm_encoder *encoder,
>>> + int encoder_type)
>>> +{
>>> + int ret;
>>> +
>>> + ret = drm_encoder_init(dev, encoder, &drm_simple_encoder_funcs_cleanup,
>>> + encoder_type, NULL);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return drmm_add_action_or_reset(dev, drmm_encoder_cleanup, encoder);
>>> +}
>>> +EXPORT_SYMBOL(drmm_simple_encoder_init);
>>> +
>>> static enum drm_mode_status
>>> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
>>> const struct drm_display_mode *mode)
>>> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
>>> index a026375464ff..27f0915599c8 100644
>>> --- a/include/drm/drm_simple_kms_helper.h
>>> +++ b/include/drm/drm_simple_kms_helper.h
>>> @@ -185,4 +185,8 @@ int drm_simple_encoder_init(struct drm_device *dev,
>>> struct drm_encoder *encoder,
>>> int encoder_type);
>>>
>>> +int drmm_simple_encoder_init(struct drm_device *dev,
>>> + struct drm_encoder *encoder,
>>> + int encoder_type);
>>> +
>>> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
>>>
>>
>> --
>> 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
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
