Re: [PATCH] drm/framebuffer: Expose only modifiers that support at least a format

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 05, 2018 at 06:44:34PM -0800, Dhinakaran Pandiyan wrote:
> Allows drivers to pass a larger modifier array, thereby avoiding
> declarations of static modifier arrays that are only slight different
> for each plane.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 1fa98bd12003..1546ffbf8e36 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -179,8 +179,8 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     const char *name, ...)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> -	unsigned int format_modifier_count = 0;
> -	int ret;
> +	unsigned int format_modifier_count, in_modifier_count = 0;
> +	int ret, i;
>  
>  	/* plane index is used with 32bit bitmasks */
>  	if (WARN_ON(config->num_total_plane >= 32))
> @@ -216,22 +216,43 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (format_modifiers) {
>  		const uint64_t *temp_modifiers = format_modifiers;
> +
>  		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> -			format_modifier_count++;
> +			in_modifier_count++;
>  	}
>  
> -	plane->modifier_count = format_modifier_count;
> -	plane->modifiers = kmalloc_array(format_modifier_count,
> +	plane->modifiers = kmalloc_array(in_modifier_count,
>  					 sizeof(format_modifiers[0]),
>  					 GFP_KERNEL);
>  
> -	if (format_modifier_count && !plane->modifiers) {
> +	if (in_modifier_count && !plane->modifiers) {
>  		DRM_DEBUG_KMS("out of memory when allocating plane\n");
>  		kfree(plane->format_types);
>  		drm_mode_object_unregister(dev, &plane->base);
>  		return -ENOMEM;
>  	}
>  
> +	for (i = 0, format_modifier_count = 0; i < in_modifier_count; i++) {
> +		int j;
> +
> +		for (j = 0; funcs->format_mod_supported && j < format_count; j++)
> +			if (funcs->format_mod_supported(plane, formats[j],
> +							format_modifiers[i]))
> +				break;
> +
> +		if (j < format_count)
> +			plane->modifiers[format_modifier_count++] =
> +				format_modifiers[i];
> +	}
> +
> +	if (format_modifier_count < in_modifier_count) {
> +		size_t size;
> +
> +		size = format_modifier_count * sizeof(format_modifiers[0]);
> +		plane->modifiers = krealloc(plane->modifiers, size, GFP_KERNEL);

Should check that the realloc actually succeeded.

And I think we might want to give this same treatment to plane->formats[]
as well.

And perhaps we could even throw out plane->modifiers[] and just rely on
the IN_FORMATS blob exclusively? Hmm. Looks like that is not getting fully
populated unless the driver has provided .format_mod_supported(). Not
sure why that is, and not sure what userspace is supposed to do with a
partially filled blob like that. I'm thinking we shouldn't even attach
the property to the plane in that case.

> +	}
> +	plane->modifier_count = format_modifier_count;
> +
>  	if (name) {
>  		va_list ap;
>  
> @@ -251,8 +272,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
>  	plane->format_count = format_count;
> -	memcpy(plane->modifiers, format_modifiers,
> -	       format_modifier_count * sizeof(format_modifiers[0]));
>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> -- 
> 2.14.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx