On Thu, Jul 23, 2020 at 11:13 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> 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.
This is fine for tiny drivers, it's totally non-workable for big
drivers where the number of encoder/connector we need are very dynamic
(depending upon which platform you run on). Try doing this with admgpu
or some similar complex driver, it just doesn't work.
-Daniel
>
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
