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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> > ---
> >  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);
 /* 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?
 
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux