Re: [PATCH v3 1/5] drm: Introduce plane and CRTC scaling filter properties

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Bharadiya,Pankaj" <pankaj.laxminarayan.bharadiya@intel.com>
Cc: daniels@collabora.com, David Airlie <airlied@linux.ie>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 1/5] drm: Introduce plane and CRTC scaling filter properties
Date: Wed, 8 Apr 2020 16:35:54 +0300	[thread overview]
Message-ID: <20200408133554.GR6112@intel.com> (raw)
In-Reply-To: <20200408094726.GA9393@plaxmina-desktop.iind.intel.com>

On Wed, Apr 08, 2020 at 03:17:27PM +0530, Bharadiya,Pankaj wrote:
> On Tue, Apr 07, 2020 at 08:28:02PM +0300, Ville Syrjälä wrote:
> > On Tue, Mar 31, 2020 at 12:08:53AM +0530, Pankaj Bharadiya wrote:
> > > Introduce per-plane and per-CRTC scaling filter properties to allow
> > > userspace to select the driver's default scaling filter or
> > > Nearest-neighbor(NN) filter for upscaling operations on CRTC and
> > > plane.
> > > 
> > > Drivers can set up this property for a plane by calling
> > > drm_plane_create_scaling_filter() and for a CRTC by calling
> > > drm_crtc_create_scaling_filter().
> > > 
> > > NN filter works by filling in the missing color values in the upscaled
> > > image with that of the coordinate-mapped nearest source pixel value.
> > > 
> > > NN filter for integer multiple scaling can be particularly useful for
> > > for pixel art games that rely on sharp, blocky images to deliver their
> > > distinctive look.
> > > 
> > > changes since v2:
> > > * Create per-plane and per-CRTC scaling filter property (Ville)
> > > changes since v1:
> > > * None
> > > changes since RFC:
> > > * Add separate properties for plane and CRTC (Ville)
> > > 
> > > Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
> > >  drivers/gpu/drm/drm_crtc.c        | 78 +++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_plane.c       | 78 +++++++++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h            | 16 +++++++
> > >  include/drm/drm_plane.h           | 21 +++++++++
> > >  5 files changed, 201 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index a1e5e262bae2..ac7dabbf0bcf 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -469,6 +469,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > >  			return -EFAULT;
> > >  
> > >  		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
> > > +	} else if (property == crtc->scaling_filter_property) {
> > > +		state->scaling_filter = val;
> > >  	} else if (crtc->funcs->atomic_set_property) {
> > >  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> > >  	} else {
> > > @@ -503,6 +505,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > >  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > >  	else if (property == config->prop_out_fence_ptr)
> > >  		*val = 0;
> > > +	else if (property == crtc->scaling_filter_property)
> > 
> > Random side observation: Why do we have two different styles to naming
> > these things (prop_foo vs. foo_property)? Would be nice to unify this
> > one way or the other.
> 
> Need to handle this separately.
> 
> All per-plane props follow foo_property convention and we have mixed 
> conventions for properties in struct drm_mode_config with majority being
> foo_property.
> 
> > 
> > > +		*val = state->scaling_filter;
> > >  	else if (crtc->funcs->atomic_get_property)
> > >  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> > >  	else
> > > @@ -583,6 +587,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> > >  					sizeof(struct drm_rect),
> > >  					&replaced);
> > >  		return ret;
> > > +	} else if (property == plane->scaling_filter_property) {
> > > +		state->scaling_filter = val;
> > >  	} else if (plane->funcs->atomic_set_property) {
> > >  		return plane->funcs->atomic_set_property(plane, state,
> > >  				property, val);
> > > @@ -641,6 +647,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > >  	} else if (property == config->prop_fb_damage_clips) {
> > >  		*val = (state->fb_damage_clips) ?
> > >  			state->fb_damage_clips->base.id : 0;
> > > +	} else if (property == plane->scaling_filter_property) {
> > > +		*val = state->scaling_filter;
> > >  	} else if (plane->funcs->atomic_get_property) {
> > >  		return plane->funcs->atomic_get_property(plane, state, property, val);
> > >  	} else {
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 4936e1080e41..95502c88966b 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -748,3 +748,81 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
> > >  
> > >  	return ret;
> > >  }
> > > +
> > > +/**
> > > + * DOC: CRTC scaling filter property
> > > + *
> > > + * SCALING_FILTER:
> > > + *
> > > + *	Indicates scaling filter to be used for CRTC scaler
> > > + *
> > > + *	The value of this property can be one of the following:
> > > + *	Default:
> > > + *		Driver's default scaling filter
> > > + *	Nearest Neighbor:
> > > + *		Nearest Neighbor scaling filter
> > > + *
> > > + * Drivers can set up this property for a CRTC by calling
> > > + * drm_crtc_create_scaling_filter_property
> > > + */
> > > +
> > > +/**
> > > + * drm_crtc_create_scaling_filter_property - create a new scaling filter
> > > + * property
> > > + *
> > > + * @crtc: drm CRTC
> > > + * @supported_filters: bitmask of supported scaling filters, must include
> > > + *		       BIT(DRM_SCALING_FILTER_DEFAULT).
> > > + *
> > > + * This function lets driver to enable the scaling filter property on a given
> > > + * CRTC.
> > > + *
> > > + * RETURNS:
> > > + * Zero for success or -errno
> > > + */
> > > +int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
> > > +					    unsigned int supported_filters)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_property *prop;
> > > +	static const struct drm_prop_enum_list props[] = {
> > > +		{ DRM_SCALING_FILTER_DEFAULT, "Default" },
> > > +		{ DRM_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
> > > +	};
> > > +	unsigned int valid_mode_mask = BIT(DRM_SCALING_FILTER_DEFAULT) |
> > > +				       BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR);
> > > +	int i;
> > > +
> > > +	if (WARN_ON((supported_filters & ~valid_mode_mask) ||
> > > +		    ((supported_filters & BIT(DRM_SCALING_FILTER_DEFAULT)) == 0)))
> > > +		return -EINVAL;
> > > +
> > > +	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> > > +				   "SCALING_FILTER",
> > > +				   hweight32(supported_filters));
> > > +	if (!prop)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> > > +		int ret;
> > > +
> > > +		if (!(BIT(props[i].type) & supported_filters))
> > > +			continue;
> > > +
> > > +		ret = drm_property_add_enum(prop, props[i].type,
> > > +					    props[i].name);
> > > +
> > > +		if (ret) {
> > > +			drm_property_destroy(dev, prop);
> > > +
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	drm_object_attach_property(&crtc->base, prop,
> > > +				   DRM_SCALING_FILTER_DEFAULT);
> > 
> > Everything up to here is identical between the crtc and plane. Needs a
> > refactoring. In fact this whole thing seems pretty generic.
> 
> How about spliting code like below -
> 
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -72,6 +72,9 @@ int drm_crtc_force_disable(struct drm_crtc *crtc);
>  
>  struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
>  
> +struct drm_property *
> +drm_prepare_scaling_filter_prop(struct drm_device *dev,
> +				unsigned int supported_filters);

s/prepare/create/
with that seems good enough.

>  /* IOCTLs */
>  int drm_mode_getcrtc(struct drm_device *dev,
>  		     void *data, struct drm_file *file_priv);
> 
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..e63614fe3eed 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1221,3 +1221,93 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  
>  	return ret;
>  }
> +
> +struct drm_property *
> +drm_prepare_scaling_filter_prop(struct drm_device *dev,
> +				unsigned int supported_filters)
> +{
> +	struct drm_property *prop;
> +	static const struct drm_prop_enum_list props[] = {
> +		{ DRM_SCALING_FILTER_DEFAULT, "Default" },
> +		{ DRM_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
> +	};
> +	unsigned int valid_mode_mask = BIT(DRM_SCALING_FILTER_DEFAULT) |
> +				       BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR);
> +	int i;
> +
> +	if (WARN_ON((supported_filters & ~valid_mode_mask) ||
> +		    ((supported_filters & BIT(DRM_SCALING_FILTER_DEFAULT)) == 0)))
> +		return ERR_PTR(-EINVAL);
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> +				   "SCALING_FILTER",
> +				   hweight32(supported_filters));
> +	if (!prop)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		int ret;
> +
> +		if (!(BIT(props[i].type) & supported_filters))
> +			continue;
> +
> +		ret = drm_property_add_enum(prop, props[i].type,
> +					    props[i].name);
> +
> +		if (ret) {
> +			drm_property_destroy(dev, prop);
> +
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	return prop;
> +}
> +
> +/**
> + * drm_plane_create_scaling_filter_property - create a new scaling filter
> + * property
> + *
> + * @plane: drm plane
> + * @supported_filters: bitmask of supported scaling filters, must include
> + *		       BIT(DRM_SCALING_FILTER_DEFAULT).
> + *
> + * This function lets driver to enable the scaling filter property on a given
> + * plane.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
> +					     unsigned int supported_filters)
> +{
> +	struct drm_property *prop =
> +		drm_prepare_scaling_filter_prop(plane->dev, supported_filters);
> +
> +	if (IS_ERR(prop))
> +		return PTR_ERR(prop);
> +
> +	drm_object_attach_property(&plane->base, prop,
> +				   DRM_SCALING_FILTER_DEFAULT);
> +	plane->scaling_filter_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
> 
> index 4936e1080e41..b48e0bce8f60 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> 
> +int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
> +					    unsigned int supported_filters)
> +{
> +	struct drm_property *prop =
> +		drm_prepare_scaling_filter_prop(crtc->dev, supported_filters);
> +
> +	if (IS_ERR(prop))
> +		return PTR_ERR(prop);
> +
> +	drm_object_attach_property(&crtc->base, prop,
> +				   DRM_SCALING_FILTER_DEFAULT);
> +	crtc->scaling_filter_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
> 
> 
> > Should probably think about just adding that bitmask to
> > drm_property_create_enum(). I suppose we could try to avoid having to
> > change all the existing callers by keeping the current thing without the
> > bitmask (though it could probably internally just call the version which
> > takes the bitmask, assuming our enum values aren't too big for that.
> 
> As more filters can be added in future and different hardwares can have
> different capabilities, I think it make sense to provide a bitmask to the
> callers so that drivers can expose *only* filters which they support.
> 
> What do you think?

I was musing about something like 

drm_property_create_enum(...
+	supported_bitmask
	);

Nothing specifically about the scaling filter prop.

>  
> Thanks,
> Pankaj
> 
> > 
> > Otherwise the patch seems reasonable.
> > 
> > > +	crtc->scaling_filter_property = prop;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> 
> [snip]
> >  					state->fb_damage_clips->data : NULL);
> > >  }
> > >  
> > > +int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
> > > +					     unsigned int supported_filters);
> > > +
> > >  #endif
> > > -- 
> > > 2.23.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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