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

[Dhinakaran Pandiyan]

On Tue, 2018-11-06 at 16:13 +0200, Ville Syrjälä wrote:
> 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 at lists.freedesktop.org
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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_modifier
> > s[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.
Didn't see a failure path for new size smaller than old, the return is
the same pointer passed to krealloc().

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

Shouldn't it copy the modifier array into the blob and mark all formats
as supported? drm_plane_check_pixel_format() seems to allow any valid
format for a modifier in this case.


-DK

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