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); +} + +/** + * 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. + * 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. + * + * 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 */
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.
You can already embed the encoder in another structure and things should work as expected.
Best regards Thomas
+/**
- 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.
- 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.
- 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 */
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().
regards Philipp
On Wed, Jul 22, 2020 at 05:08:03PM +0200, Philipp Zabel wrote:
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.
Yeah the drmm_ versions of these need to check that the ->cleanup hook is NULL.
Also there's not actually a double-free, since drm_foo_cleanup removes it from the lists, which means drm_mode_config_cleanup won't even see it. But if the driver has some additional code in ->cleanup that won't ever run, so probably still a bug.
I also think that the drmm_foo_ wrappers should also do the allocation (and upcasting) kinda like drmm_dev_alloc(). Otherwise we're still stuck with tons of boilerplate.
For now I think it's ok if drivers that switch to drmm_ just copypaste, until we're sure this is the right thing to do. And then maybe also roll these out for all objects that stay for the entire lifetime of drm_device (plane, crtc, encoder, plus variants). Just to make sure we're consistent across all of them. -Daniel
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().
regards Philipp _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel,
On Thu, 2020-07-23 at 00:22 +0200, daniel@ffwll.ch wrote: [...]
Yeah the drmm_ versions of these need to check that the ->cleanup hook is NULL.
Also there's not actually a double-free, since drm_foo_cleanup removes it from the lists, which means drm_mode_config_cleanup won't even see it. But if the driver has some additional code in ->cleanup that won't ever run, so probably still a bug.
I also think that the drmm_foo_ wrappers should also do the allocation (and upcasting) kinda like drmm_dev_alloc(). Otherwise we're still stuck with tons of boilerplate.
Ok, I'll try this:
drmm_encoder_init() variant can verify that the passed drm_encoder_funcs::destroy hook is NULL.
drmm_simple_encoder_init() can just provide empty drm_encoder_funcs internally.
For now I think it's ok if drivers that switch to drmm_ just copypaste, until we're sure this is the right thing to do. And then maybe also roll these out for all objects that stay for the entire lifetime of drm_device (plane, crtc, encoder, plus variants). Just to make sure we're consistent across all of them.
Thank you for clarifying, I wasn't sure this was the goal. I've started with this function mostly because this is the most used one in imx-drm and the only one where I didn't have to deal with va_args boilerplate.
regards Philipp
On Thu, Jul 23, 2020 at 4:46 PM Philipp Zabel p.zabel@pengutronix.de wrote:
Hi Daniel,
On Thu, 2020-07-23 at 00:22 +0200, daniel@ffwll.ch wrote: [...]
Yeah the drmm_ versions of these need to check that the ->cleanup hook is NULL.
Also there's not actually a double-free, since drm_foo_cleanup removes it from the lists, which means drm_mode_config_cleanup won't even see it. But if the driver has some additional code in ->cleanup that won't ever run, so probably still a bug.
I also think that the drmm_foo_ wrappers should also do the allocation (and upcasting) kinda like drmm_dev_alloc(). Otherwise we're still stuck with tons of boilerplate.
Ok, I'll try this:
drmm_encoder_init() variant can verify that the passed drm_encoder_funcs::destroy hook is NULL.
drmm_simple_encoder_init() can just provide empty drm_encoder_funcs internally.
For now I think it's ok if drivers that switch to drmm_ just copypaste, until we're sure this is the right thing to do. And then maybe also roll these out for all objects that stay for the entire lifetime of drm_device (plane, crtc, encoder, plus variants). Just to make sure we're consistent across all of them.
Thank you for clarifying, I wasn't sure this was the goal. I've started with this function mostly because this is the most used one in imx-drm and the only one where I didn't have to deal with va_args boilerplate.
Hm if we go with also exposing the drmm_foo_init() variants then I think these should check that the passed-in memory is indeed allocated by drmres on the right device. That's kinda the bug I'm afraid of when we exposed drmm_foo_init() to drivers and not just drmm_foo_alloc() which does everything and correctly for you. For the drmm_is_manged or so I think we can just reuse the same loop I've typed up already for drmm_kfree. There shouldn't be too many allocations on that list that the list walk at driver load will matter. Oh also better name than drmm_is_managed would be good, devres doesn't seem to have that. Hm maybe drmm_assert_managed(dev, void, size) and it checks that it's fully contained within a drmm_kmalloc region.
Cheers, Daniel
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
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.
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)
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 */
On Thu, Jul 23, 2020 at 9:36 AM Thomas Zimmermann tzimmermann@suse.de 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 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@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.
+}
+/**
- 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
Am 23.07.20 um 11:05 schrieb Daniel Vetter:
On Thu, Jul 23, 2020 at 9:36 AM Thomas Zimmermann tzimmermann@suse.de 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@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.
+}
+/**
- 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 23, 2020 at 11:13 AM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 23.07.20 um 11:05 schrieb Daniel Vetter:
On Thu, Jul 23, 2020 at 9:36 AM Thomas Zimmermann tzimmermann@suse.de 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@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.
+}
+/**
- 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@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
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@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.
+}
+/**
- 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.
- 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.
regards Philipp
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@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thomas,
On Fri, 2020-07-24 at 09:17 +0200, Thomas Zimmermann wrote: [...]
@@ -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.
Alright, I'll add a
if (WARN_ON(!encoder->dev)) return;
then.
[...]
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.
They certainly are. How about "drivers that can embed encoders in a preallocated structure should use drm_simple_encoder_init() instead"? The difference between the two will be clearer when the actual allocation is folded into drmm_simple_encoder_init() as well.
regards Philipp
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
daniel＠ffwll.ch
-
Philipp Zabel -
Thomas Zimmermann