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
