thr3ads.net - Linux Virtualization - [PATCH 2/6] drm: Add drm_simple_encoder

If this information is useful, please help other people find it:
Share via:

Daniel Vetter

2020-Feb-07 13:37 UTC

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

On Fri, Feb 07, 2020 at 09:41:31AM +0100, Thomas Zimmermann
wrote:> The simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
This has quick a bit midlayer taste to it ... I think having this as a
helper would be cleaner ...

The other bit is drm_encoder->possible_crtcs. If we do create a helper for
these, lets at least try to make them not suck too badly :-) Otherwise I
guess it would be time to officially document what exactly possible_crtcs
== 0 means from an uabi pov.
-Daniel
> ---
>  drivers/gpu/drm/drm_encoder.c | 116 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_encoder.h     |  10 +++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index ffe691a1bf34..1a65cab1f310 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -178,6 +178,122 @@ int drm_encoder_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_encoder_init);
>  
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +/**
> + * drm_simple_encoder_init - Init a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL
> + *        for default name
> + *
> + * Initialises a preallocated encoder that has no further functionality.
The
> + * encoder will be released automatically.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> +			    struct drm_encoder *encoder,
> +			    int encoder_type, const char *name, ...)
> +{
> +	char *namestr = NULL;
> +	int ret;
> +
> +	if (name) {
> +		va_list ap;
> +
> +		va_start(ap, name);
> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
> +		va_end(ap);
> +		if (!namestr)
> +			return -ENOMEM;
> +	}
> +
> +	ret = __drm_encoder_init(dev, encoder,
> +				 &drm_simple_encoder_funcs_cleanup,
> +				 encoder_type, namestr);
> +	if (ret)
> +		goto err_kfree;
> +
> +	return 0;
> +
> +err_kfree:
> +	if (name)
> +		kfree(namestr);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +
> +	drm_encoder_cleanup(encoder);
> +	devm_kfree(dev->dev, encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> +	.destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for
> + *        default name
> + *
> + * Allocates and initialises an encoder that has no further functionality.
The
> + * encoder will be released automatically.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> +					      int encoder_type,
> +					      const char *name, ...)
> +{
> +	char *namestr = NULL;
> +	struct drm_encoder *encoder;
> +	int ret;
> +
> +	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
> +	if (!encoder)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (name) {
> +		va_list ap;
> +
> +		va_start(ap, name);
> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
> +		va_end(ap);
> +		if (!namestr) {
> +			ret = -ENOMEM;
> +			goto err_devm_kfree;
> +		}
> +	}
> +
> +	ret = __drm_encoder_init(dev, encoder,
> +				 &drm_simple_encoder_funcs_destroy,
> +				 encoder_type, namestr);
> +	if (ret)
> +		goto err_kfree;
> +
> +	return encoder;
> +
> +err_kfree:
> +	if (name)
> +		kfree(namestr);
> +err_devm_kfree:
> +	devm_kfree(dev->dev, encoder);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
>  /**
>   * drm_encoder_cleanup - cleans up an initialised encoder
>   * @encoder: encoder to cleanup
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 5623994b6e9e..0214f6cf9de6 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -190,6 +190,16 @@ int drm_encoder_init(struct drm_device *dev,
>  		     const struct drm_encoder_funcs *funcs,
>  		     int encoder_type, const char *name, ...);
>  
> +__printf(4, 5)
> +int drm_simple_encoder_init(struct drm_device *dev,
> +			    struct drm_encoder *encoder,
> +			    int encoder_type, const char *name, ...);
> +
> +__printf(3, 4)
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> +					      int encoder_type,
> +					      const char *name, ...);
> +
>  /**
>   * drm_encoder_index - find the index of a registered encoder
>   * @encoder: encoder to find index for
> -- 
> 2.25.0
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Ville Syrjälä

2020-Feb-07 14:00 UTC

head link

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

On Fri, Feb 07, 2020 at 02:37:20PM +0100, Daniel Vetter
wrote:> On Fri, Feb 07, 2020 at 09:41:31AM +0100, Thomas Zimmermann wrote:
> > The simple-encoder helpers initialize an encoder with an empty
> > implementation. This covers the requirements of most of the existing
> > DRM drivers. A call to drm_simple_encoder_create() allocates and
> > initializes an encoder instance, a call to drm_simple_encoder_init()
> > initializes a pre-allocated instance.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> 
> This has quick a bit midlayer taste to it ... I think having this as a
> helper would be cleaner ...
> 
> The other bit is drm_encoder->possible_crtcs. If we do create a helper
for
> these, lets at least try to make them not suck too badly :-) Otherwise I
> guess it would be time to officially document what exactly possible_crtcs
> == 0 means from an uabi pov.
I had some improvements for this stuff. Just re-sent the remainder.

-- 
Ville Syrj?l?
Intel

Thomas Zimmermann

2020-Feb-07 14:02 UTC

head link

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

Hi

Am 07.02.20 um 14:37 schrieb Daniel Vetter:> On Fri, Feb 07, 2020 at 09:41:31AM +0100, Thomas Zimmermann wrote:
>> The simple-encoder helpers initialize an encoder with an empty
>> implementation. This covers the requirements of most of the existing
>> DRM drivers. A call to drm_simple_encoder_create() allocates and
>> initializes an encoder instance, a call to drm_simple_encoder_init()
>> initializes a pre-allocated instance.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> 
> This has quick a bit midlayer taste to it ... I think having this as a
> helper would be cleaner ...
How would such a helper roughly look like?

Best regards
Thomas
> 
> The other bit is drm_encoder->possible_crtcs. If we do create a helper
for
> these, lets at least try to make them not suck too badly :-) Otherwise I
> guess it would be time to officially document what exactly possible_crtcs
> == 0 means from an uabi pov.
> -Daniel
> 
>> ---
>>  drivers/gpu/drm/drm_encoder.c | 116 ++++++++++++++++++++++++++++++++++
>>  include/drm/drm_encoder.h     |  10 +++
>>  2 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_encoder.c
b/drivers/gpu/drm/drm_encoder.c
>> index ffe691a1bf34..1a65cab1f310 100644
>> --- a/drivers/gpu/drm/drm_encoder.c
>> +++ b/drivers/gpu/drm/drm_encoder.c
>> @@ -178,6 +178,122 @@ int drm_encoder_init(struct drm_device *dev,
>>  }
>>  EXPORT_SYMBOL(drm_encoder_init);
>>  
>> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup
= {
>> +	.destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/**
>> + * drm_simple_encoder_init - Init a preallocated encoder
>> + * @dev: drm device
>> + * @funcs: callbacks for this encoder
>> + * @encoder_type: user visible type of the encoder
>> + * @name: printf style format string for the encoder name, or NULL
>> + *        for default name
>> + *
>> + * Initialises a preallocated encoder that has no further
functionality. The
>> + * encoder will be released automatically.
>> + *
>> + * Returns:
>> + * Zero on success, error code on failure.
>> + */
>> +int drm_simple_encoder_init(struct drm_device *dev,
>> +			    struct drm_encoder *encoder,
>> +			    int encoder_type, const char *name, ...)
>> +{
>> +	char *namestr = NULL;
>> +	int ret;
>> +
>> +	if (name) {
>> +		va_list ap;
>> +
>> +		va_start(ap, name);
>> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
>> +		va_end(ap);
>> +		if (!namestr)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	ret = __drm_encoder_init(dev, encoder,
>> +				 &drm_simple_encoder_funcs_cleanup,
>> +				 encoder_type, namestr);
>> +	if (ret)
>> +		goto err_kfree;
>> +
>> +	return 0;
>> +
>> +err_kfree:
>> +	if (name)
>> +		kfree(namestr);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_simple_encoder_init);
>> +
>> +static void drm_encoder_destroy(struct drm_encoder *encoder)
>> +{
>> +	struct drm_device *dev = encoder->dev;
>> +
>> +	drm_encoder_cleanup(encoder);
>> +	devm_kfree(dev->dev, encoder);
>> +}
>> +
>> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy
= {
>> +	.destroy = drm_encoder_destroy,
>> +};
>> +
>> +/**
>> + * drm_simple_encoder_create - Allocate and initialize an encoder
>> + * @dev: drm device
>> + * @encoder_type: user visible type of the encoder
>> + * @name: printf style format string for the encoder name, or NULL for
>> + *        default name
>> + *
>> + * Allocates and initialises an encoder that has no further
functionality. The
>> + * encoder will be released automatically.
>> + *
>> + * Returns:
>> + * The encoder on success, a pointer-encoder error code on failure.
>> + */
>> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
>> +					      int encoder_type,
>> +					      const char *name, ...)
>> +{
>> +	char *namestr = NULL;
>> +	struct drm_encoder *encoder;
>> +	int ret;
>> +
>> +	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
>> +	if (!encoder)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	if (name) {
>> +		va_list ap;
>> +
>> +		va_start(ap, name);
>> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
>> +		va_end(ap);
>> +		if (!namestr) {
>> +			ret = -ENOMEM;
>> +			goto err_devm_kfree;
>> +		}
>> +	}
>> +
>> +	ret = __drm_encoder_init(dev, encoder,
>> +				 &drm_simple_encoder_funcs_destroy,
>> +				 encoder_type, namestr);
>> +	if (ret)
>> +		goto err_kfree;
>> +
>> +	return encoder;
>> +
>> +err_kfree:
>> +	if (name)
>> +		kfree(namestr);
>> +err_devm_kfree:
>> +	devm_kfree(dev->dev, encoder);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL(drm_simple_encoder_create);
>> +
>>  /**
>>   * drm_encoder_cleanup - cleans up an initialised encoder
>>   * @encoder: encoder to cleanup
>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>> index 5623994b6e9e..0214f6cf9de6 100644
>> --- a/include/drm/drm_encoder.h
>> +++ b/include/drm/drm_encoder.h
>> @@ -190,6 +190,16 @@ int drm_encoder_init(struct drm_device *dev,
>>  		     const struct drm_encoder_funcs *funcs,
>>  		     int encoder_type, const char *name, ...);
>>  
>> +__printf(4, 5)
>> +int drm_simple_encoder_init(struct drm_device *dev,
>> +			    struct drm_encoder *encoder,
>> +			    int encoder_type, const char *name, ...);
>> +
>> +__printf(3, 4)
>> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
>> +					      int encoder_type,
>> +					      const char *name, ...);
>> +
>>  /**
>>   * drm_encoder_index - find the index of a registered encoder
>>   * @encoder: encoder to find index for
>> -- 
>> 2.25.0
>>
> 
-- 
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20200207/192ff0d7/attachment.sig>

Daniel Vetter

2020-Feb-07 16:26 UTC

head link

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

On Fri, Feb 07, 2020 at 03:02:00PM +0100, Thomas Zimmermann
wrote:> Hi
> 
> Am 07.02.20 um 14:37 schrieb Daniel Vetter:
> > On Fri, Feb 07, 2020 at 09:41:31AM +0100, Thomas Zimmermann wrote:
> >> The simple-encoder helpers initialize an encoder with an empty
> >> implementation. This covers the requirements of most of the
existing
> >> DRM drivers. A call to drm_simple_encoder_create() allocates and
> >> initializes an encoder instance, a call to
drm_simple_encoder_init()
> >> initializes a pre-allocated instance.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> > 
> > This has quick a bit midlayer taste to it ... I think having this as a
> > helper would be cleaner ...
> 
> How would such a helper roughly look like?
Essentially same code, but stuff the helper into drm-kms-helper.ko, then
make sure it still works. The separate kernel module makes sure that the
drm core and helper stuff aren't too close friends with each another :-)
Essentially like the simple display pipe helpers.
-Daniel
> 
> Best regards
> Thomas
> 
> > 
> > The other bit is drm_encoder->possible_crtcs. If we do create a
helper for
> > these, lets at least try to make them not suck too badly :-) Otherwise
I
> > guess it would be time to officially document what exactly
possible_crtcs
> > == 0 means from an uabi pov.
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/drm_encoder.c | 116
++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_encoder.h     |  10 +++
> >>  2 files changed, 126 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_encoder.c
b/drivers/gpu/drm/drm_encoder.c
> >> index ffe691a1bf34..1a65cab1f310 100644
> >> --- a/drivers/gpu/drm/drm_encoder.c
> >> +++ b/drivers/gpu/drm/drm_encoder.c
> >> @@ -178,6 +178,122 @@ int drm_encoder_init(struct drm_device *dev,
> >>  }
> >>  EXPORT_SYMBOL(drm_encoder_init);
> >>  
> >> +static const struct drm_encoder_funcs
drm_simple_encoder_funcs_cleanup = {
> >> +	.destroy = drm_encoder_cleanup,
> >> +};
> >> +
> >> +/**
> >> + * drm_simple_encoder_init - Init a preallocated encoder
> >> + * @dev: drm device
> >> + * @funcs: callbacks for this encoder
> >> + * @encoder_type: user visible type of the encoder
> >> + * @name: printf style format string for the encoder name, or
NULL
> >> + *        for default name
> >> + *
> >> + * Initialises a preallocated encoder that has no further
functionality. The
> >> + * encoder will be released automatically.
> >> + *
> >> + * Returns:
> >> + * Zero on success, error code on failure.
> >> + */
> >> +int drm_simple_encoder_init(struct drm_device *dev,
> >> +			    struct drm_encoder *encoder,
> >> +			    int encoder_type, const char *name, ...)
> >> +{
> >> +	char *namestr = NULL;
> >> +	int ret;
> >> +
> >> +	if (name) {
> >> +		va_list ap;
> >> +
> >> +		va_start(ap, name);
> >> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
> >> +		va_end(ap);
> >> +		if (!namestr)
> >> +			return -ENOMEM;
> >> +	}
> >> +
> >> +	ret = __drm_encoder_init(dev, encoder,
> >> +				 &drm_simple_encoder_funcs_cleanup,
> >> +				 encoder_type, namestr);
> >> +	if (ret)
> >> +		goto err_kfree;
> >> +
> >> +	return 0;
> >> +
> >> +err_kfree:
> >> +	if (name)
> >> +		kfree(namestr);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL(drm_simple_encoder_init);
> >> +
> >> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> >> +{
> >> +	struct drm_device *dev = encoder->dev;
> >> +
> >> +	drm_encoder_cleanup(encoder);
> >> +	devm_kfree(dev->dev, encoder);
> >> +}
> >> +
> >> +static const struct drm_encoder_funcs
drm_simple_encoder_funcs_destroy = {
> >> +	.destroy = drm_encoder_destroy,
> >> +};
> >> +
> >> +/**
> >> + * drm_simple_encoder_create - Allocate and initialize an encoder
> >> + * @dev: drm device
> >> + * @encoder_type: user visible type of the encoder
> >> + * @name: printf style format string for the encoder name, or
NULL for
> >> + *        default name
> >> + *
> >> + * Allocates and initialises an encoder that has no further
functionality. The
> >> + * encoder will be released automatically.
> >> + *
> >> + * Returns:
> >> + * The encoder on success, a pointer-encoder error code on
failure.
> >> + */
> >> +struct drm_encoder *drm_simple_encoder_create(struct drm_device
*dev,
> >> +					      int encoder_type,
> >> +					      const char *name, ...)
> >> +{
> >> +	char *namestr = NULL;
> >> +	struct drm_encoder *encoder;
> >> +	int ret;
> >> +
> >> +	encoder = devm_kzalloc(dev->dev, sizeof(*encoder),
GFP_KERNEL);
> >> +	if (!encoder)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	if (name) {
> >> +		va_list ap;
> >> +
> >> +		va_start(ap, name);
> >> +		namestr = kvasprintf(GFP_KERNEL, name, ap);
> >> +		va_end(ap);
> >> +		if (!namestr) {
> >> +			ret = -ENOMEM;
> >> +			goto err_devm_kfree;
> >> +		}
> >> +	}
> >> +
> >> +	ret = __drm_encoder_init(dev, encoder,
> >> +				 &drm_simple_encoder_funcs_destroy,
> >> +				 encoder_type, namestr);
> >> +	if (ret)
> >> +		goto err_kfree;
> >> +
> >> +	return encoder;
> >> +
> >> +err_kfree:
> >> +	if (name)
> >> +		kfree(namestr);
> >> +err_devm_kfree:
> >> +	devm_kfree(dev->dev, encoder);
> >> +	return ERR_PTR(ret);
> >> +}
> >> +EXPORT_SYMBOL(drm_simple_encoder_create);
> >> +
> >>  /**
> >>   * drm_encoder_cleanup - cleans up an initialised encoder
> >>   * @encoder: encoder to cleanup
> >> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> >> index 5623994b6e9e..0214f6cf9de6 100644
> >> --- a/include/drm/drm_encoder.h
> >> +++ b/include/drm/drm_encoder.h
> >> @@ -190,6 +190,16 @@ int drm_encoder_init(struct drm_device *dev,
> >>  		     const struct drm_encoder_funcs *funcs,
> >>  		     int encoder_type, const char *name, ...);
> >>  
> >> +__printf(4, 5)
> >> +int drm_simple_encoder_init(struct drm_device *dev,
> >> +			    struct drm_encoder *encoder,
> >> +			    int encoder_type, const char *name, ...);
> >> +
> >> +__printf(3, 4)
> >> +struct drm_encoder *drm_simple_encoder_create(struct drm_device
*dev,
> >> +					      int encoder_type,
> >> +					      const char *name, ...);
> >> +
> >>  /**
> >>   * drm_encoder_index - find the index of a registered encoder
> >>   * @encoder: encoder to find index for
> >> -- 
> >> 2.25.0
> >>
> > 
> 
> -- 
> 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

Reasonably Related Threads

Search for more reasonably related threads

Linux Virtualization - Feb 2020 - [PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

[PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

Reasonably Related Threads