[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[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);
+}
+
+/**
+ * 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 */
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3895 bytes --]

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 */
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[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().

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

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[daniel]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3109 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4457 bytes --]

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 */
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[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
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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6165 bytes --]

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
> 
> 
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Daniel Vetter]

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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[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.

> > +}
> > +
> > +/**
> > + * 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Philipp Zabel]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5399 bytes --]

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
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Philipp Zabel]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel