[PATCH v3 0/5] Introduce drm scaling filter property

[PATCH v3 0/5] Introduce drm scaling filter property

[Pankaj Bharadiya]

This series is the continuation for the RFC that I posted earlier [1]

[1] RFC: https://patchwork.freedesktop.org/series/73884/

Integer scaling (IS) is a nearest-neighbor upscaling technique that
simply scales up the existing pixels by an integer (i.e., whole
number) multiplier. Nearest-neighbor (NN) interpolation works by
filling in the missing color values in the upscaled image with that of
the coordinate-mapped nearest source pixel value.

Both IS and NN preserve the clarity of the original image. In
contrast, traditional upscaling algorithms, such as bilinear or
bicubic interpolation, result in blurry upscaled images because they
employ interpolation techniques that smooth out the transition from
one pixel to another.  Therefore, integer scaling is particularly
useful for pixel art games that rely on sharp, blocky images to
deliver their distinctive look.

Many gaming communities have been asking for integer-mode scaling
support, some links and background:

https://software.intel.com/en-us/articles/integer-scaling-support-on-intel-graphics
http://tanalin.com/en/articles/lossless-scaling/
https://community.amd.com/thread/209107
https://www.nvidia.com/en-us/geforce/forums/game-ready-drivers/13/1002/feature-request-nonblurry-upscaling-at-integer-rat/

This patch series -
  - Introduces new scaling filter properties to allow userspace to
    select  the driver's default scaling filter or
    Nearest-neighbor(NN) filter for scaling operations on crtc and plane.
  - Implements and enable integer scaling for i915

Userspace patch series link: https://github.com/lrusak/xbmc/pull/24 

Thanks to Shashank for initiating this work. His initial work can be
found here [2]

[2] https://patchwork.freedesktop.org/patch/337082/

changes since v2:
* Add per-crtc and per-plane scaling filter property (Ville)
* Rename/refoctor functions and macros.
* Duplicate the scaling filter in crtc and plane hw state (Ville)
 
changes since v1: 
* Add userspace patch link to this cover letter.
* 4/5 - Rearrange skl_scaler_setup_nearest_neighbor_filter() to iterate
  the registers directly instead of the phases and taps (Ville)

Pankaj Bharadiya (5):
  drm: Introduce plane and CRTC scaling filter properties
  drm/drm-kms.rst: Add plane and CRTC scaling filter property
    documentation
  drm/i915: Introduce scaling filter related registers and bit fields.
  drm/i915/display: Add Nearest-neighbor based integer scaling support
  drm/i915: Enable scaling filter for plane and CRTC

 Documentation/gpu/drm-kms.rst                 |  12 ++
 drivers/gpu/drm/drm_atomic_uapi.c             |   8 ++
 drivers/gpu/drm/drm_crtc.c                    |  78 ++++++++++++
 drivers/gpu/drm/drm_plane.c                   |  78 ++++++++++++
 .../gpu/drm/i915/display/intel_atomic_plane.c |   1 +
 drivers/gpu/drm/i915/display/intel_display.c  | 116 +++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_display.h  |   4 +
 .../drm/i915/display/intel_display_types.h    |   2 +
 drivers/gpu/drm/i915/display/intel_sprite.c   |  15 ++-
 drivers/gpu/drm/i915/i915_reg.h               |  22 ++++
 include/drm/drm_crtc.h                        |  16 +++
 include/drm/drm_plane.h                       |  21 ++++
 12 files changed, 369 insertions(+), 4 deletions(-)

-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pankaj Bharadiya]

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)
+		*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);
+	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
index d6ad60ab0d38..a6fc95942851 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1221,3 +1221,81 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 	return ret;
 }
+
+/**
+ * DOC: Plane scaling filter property
+ *
+ * SCALING_FILTER:
+ *
+ *	Indicates scaling filter to be used for plane 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 plane by calling
+ * drm_plane_create_scaling_filter_property
+ */
+
+/**
+ * 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_device *dev = plane->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(&plane->base, prop,
+				   DRM_SCALING_FILTER_DEFAULT);
+	plane->scaling_filter_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 59b51a09cae6..ba839e5e357d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -324,6 +324,13 @@ struct drm_crtc_state {
 	 */
 	bool self_refresh_active;
 
+	/**
+	 * @scaling_filter:
+	 *
+	 * Scaling filter to be applied
+	 */
+	enum drm_scaling_filter scaling_filter;
+
 	/**
 	 * @event:
 	 *
@@ -1083,6 +1090,12 @@ struct drm_crtc {
 	/** @properties: property tracking for this CRTC */
 	struct drm_object_properties properties;
 
+	/**
+	 * @scaling_filter_property: property to apply a particular filter while
+	 * scaling.
+	 */
+	struct drm_property *scaling_filter_property;
+
 	/**
 	 * @state:
 	 *
@@ -1266,4 +1279,7 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
 #define drm_for_each_crtc(crtc, dev) \
 	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
 
+int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
+					    unsigned int supported_filters);
+
 #endif /* __DRM_CRTC_H__ */
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3f396d94afe4..1d82b264e5e4 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -35,6 +35,11 @@ struct drm_crtc;
 struct drm_printer;
 struct drm_modeset_acquire_ctx;
 
+enum drm_scaling_filter {
+	DRM_SCALING_FILTER_DEFAULT,
+	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
+};
+
 /**
  * struct drm_plane_state - mutable plane state
  *
@@ -214,6 +219,13 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+	/**
+	 * @scaling_filter:
+	 *
+	 * Scaling filter to be applied
+	 */
+	enum drm_scaling_filter scaling_filter;
+
 	/**
 	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
 	 * and for async plane updates.
@@ -724,6 +736,12 @@ struct drm_plane {
 	 * See drm_plane_create_color_properties().
 	 */
 	struct drm_property *color_range_property;
+
+	/**
+	 * @scaling_filter_property: property to apply a particular filter while
+	 * scaling.
+	 */
+	struct drm_property *scaling_filter_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
@@ -862,4 +880,7 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state)
 					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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/5] drm/drm-kms.rst: Add plane and CRTC scaling filter property documentation

[Pankaj Bharadiya]

Add documentation for newly introduced KMS plane and CRTC scaling
filter properties.

changes since v1:
* None
changes since RFC:
* Add separate documentation for plane and CRTC.

Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
---
 Documentation/gpu/drm-kms.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 906771e03103..b0335e9d887c 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -509,6 +509,18 @@ Variable Refresh Properties
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
    :doc: Variable refresh properties
 
+Plane Scaling Filter Property
+-----------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_plane.c
+   :doc: Plane scaling filter property
+
+CRTC Scaling Filter Property
+-----------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
+   :doc: CRTC scaling filter property
+
 Existing KMS Properties
 -----------------------
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/5] drm/i915: Introduce scaling filter related registers and bit fields.

[Pankaj Bharadiya]

Introduce scaler registers and bit fields needed to configure the
scaling filter in prgrammed mode and configure scaling filter
coefficients.

changes since v2:
* Change macro names to CNL_* and  use +(set)*8 instead of adding
  another trip through _PICK_EVEN (Ville).
changes since v1:
* None
changes since RFC:
* Parametrize scaler coeffient macros by 'set' (Ville)

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9c53fe918be6..16b6e398b989 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7205,6 +7205,7 @@ enum {
 #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
 #define PS_FILTER_MASK         (3 << 23)
 #define PS_FILTER_MEDIUM       (0 << 23)
+#define PS_FILTER_PROGRAMMED   (1 << 23)
 #define PS_FILTER_EDGE_ENHANCE (2 << 23)
 #define PS_FILTER_BILINEAR     (3 << 23)
 #define PS_VERT3TAP            (1 << 21)
@@ -7219,6 +7220,10 @@ enum {
 #define PS_VADAPT_MODE_MOST_ADAPT  (3 << 5)
 #define PS_PLANE_Y_SEL_MASK  (7 << 5)
 #define PS_PLANE_Y_SEL(plane) (((plane) + 1) << 5)
+#define PS_Y_VERT_FILTER_SELECT(set)   ((set) << 4)
+#define PS_Y_HORZ_FILTER_SELECT(set)   ((set) << 3)
+#define PS_UV_VERT_FILTER_SELECT(set)  ((set) << 2)
+#define PS_UV_HORZ_FILTER_SELECT(set)  ((set) << 1)
 
 #define _PS_PWR_GATE_1A     0x68160
 #define _PS_PWR_GATE_2A     0x68260
@@ -7281,6 +7286,17 @@ enum {
 #define _PS_ECC_STAT_2B     0x68AD0
 #define _PS_ECC_STAT_1C     0x691D0
 
+#define _PS_COEF_SET0_INDEX_1A	   0x68198
+#define _PS_COEF_SET0_INDEX_2A	   0x68298
+#define _PS_COEF_SET0_INDEX_1B	   0x68998
+#define _PS_COEF_SET0_INDEX_2B	   0x68A98
+#define PS_COEE_INDEX_AUTO_INC	   (1 << 10)
+
+#define _PS_COEF_SET0_DATA_1A	   0x6819C
+#define _PS_COEF_SET0_DATA_2A	   0x6829C
+#define _PS_COEF_SET0_DATA_1B	   0x6899C
+#define _PS_COEF_SET0_DATA_2B	   0x68A9C
+
 #define _ID(id, a, b) _PICK_EVEN(id, a, b)
 #define SKL_PS_CTRL(pipe, id) _MMIO_PIPE(pipe,        \
 			_ID(id, _PS_1A_CTRL, _PS_2A_CTRL),       \
@@ -7309,7 +7325,13 @@ enum {
 #define SKL_PS_ECC_STAT(pipe, id)  _MMIO_PIPE(pipe,     \
 			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
 			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))
+#define CNL_PS_COEF_INDEX_SET(pipe, id, set)  _MMIO_PIPE(pipe,    \
+			_ID(id, _PS_COEF_SET0_INDEX_1A, _PS_COEF_SET0_INDEX_2A) + (set) * 8, \
+			_ID(id, _PS_COEF_SET0_INDEX_1B, _PS_COEF_SET0_INDEX_2B) + (set) * 8)
 
+#define CNL_PS_COEF_DATA_SET(pipe, id, set)  _MMIO_PIPE(pipe,     \
+			_ID(id, _PS_COEF_SET0_DATA_1A, _PS_COEF_SET0_DATA_2A) + (set) * 8, \
+			_ID(id, _PS_COEF_SET0_DATA_1B, _PS_COEF_SET0_DATA_2B) + (set) * 8)
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/5] drm/i915/display: Add Nearest-neighbor based integer scaling support

[Pankaj Bharadiya]

Integer scaling (IS) is a nearest-neighbor upscaling technique that
simply scales up the existing pixels by an integer
(i.e., whole number) multiplier.Nearest-neighbor (NN) interpolation
works by filling in the missing color values in the upscaled image
with that of the coordinate-mapped nearest source pixel value.

Both IS and NN preserve the clarity of the original image. Integer
scaling is particularly useful for pixel art games that rely on
sharp, blocky images to deliver their distinctive look.

Introduce functions to configure the scaler filter coefficients to
enable nearest-neighbor filtering.

Bspec: 49247

changes since v2:
* Move APIs from 5/5 into this patch.
* Change filter programming related function names to cnl_*, move
  filter select bits related code into inline function (Ville)
changes since v1:
* Rearrange skl_scaler_setup_nearest_neighbor_filter() to iterate the
  registers directly instead of the phases and taps (Ville)

changes since RFC:
* Refine the skl_scaler_setup_nearest_neighbor_filter() logic (Ville)

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 99 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  4 +
 2 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 37bd7ce88ecd..85c29a69519f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6237,6 +6237,105 @@ void skl_scaler_disable(const struct intel_crtc_state *old_crtc_state)
 		skl_detach_scaler(crtc, i);
 }
 
+static int cnl_coef_tap(int i)
+{
+	return i % 7;
+}
+
+static u16 cnl_nearest_filter_coef(int t)
+{
+	return t == 3 ? 0x0800 : 0x3000;
+}
+
+/**
+ *  Theory behind setting nearest-neighbor integer scaling:
+ *
+ *  17 phase of 7 taps requires 119 coefficients in 60 dwords per set.
+ *  The letter represents the filter tap (D is the center tap) and the number
+ *  represents the coefficient set for a phase (0-16).
+ *
+ *         +------------+------------------------+------------------------+
+ *         |Index value | Data value coeffient 1 | Data value coeffient 2 |
+ *         +------------+------------------------+------------------------+
+ *         |   00h      |          B0            |          A0            |
+ *         +------------+------------------------+------------------------+
+ *         |   01h      |          D0            |          C0            |
+ *         +------------+------------------------+------------------------+
+ *         |   02h      |          F0            |          E0            |
+ *         +------------+------------------------+------------------------+
+ *         |   03h      |          A1            |          G0            |
+ *         +------------+------------------------+------------------------+
+ *         |   04h      |          C1            |          B1            |
+ *         +------------+------------------------+------------------------+
+ *         |   ...      |          ...           |          ...           |
+ *         +------------+------------------------+------------------------+
+ *         |   38h      |          B16           |          A16           |
+ *         +------------+------------------------+------------------------+
+ *         |   39h      |          D16           |          C16           |
+ *         +------------+------------------------+------------------------+
+ *         |   3Ah      |          F16           |          C16           |
+ *         +------------+------------------------+------------------------+
+ *         |   3Bh      |        Reserved        |          G16           |
+ *         +------------+------------------------+------------------------+
+ *
+ *  To enable nearest-neighbor scaling:  program scaler coefficents with
+ *  the center tap (Dxx) values set to 1 and all other values set to 0 as per
+ *  SCALER_COEFFICIENT_FORMAT
+ *
+ */
+
+static void cnl_program_nearest_filter_coefs(struct drm_i915_private *dev_priv,
+					     enum pipe pipe, int id, int set)
+{
+	int i;
+
+	intel_de_write_fw(dev_priv, CNL_PS_COEF_INDEX_SET(pipe, id, set),
+			  PS_COEE_INDEX_AUTO_INC);
+
+	for (i = 0; i < 17 * 7; i += 2) {
+		u32 tmp;
+		int t;
+
+		t = cnl_coef_tap(i);
+		tmp = cnl_nearest_filter_coef(t);
+
+		t = cnl_coef_tap(i + 1);
+		tmp |= cnl_nearest_filter_coef(t) << 16;
+
+		intel_de_write_fw(dev_priv, CNL_PS_COEF_DATA_SET(pipe, id, set),
+				  tmp);
+	}
+
+	intel_de_write_fw(dev_priv, CNL_PS_COEF_INDEX_SET(pipe, id, set), 0);
+}
+
+inline u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set)
+{
+	if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
+		return (PS_FILTER_PROGRAMMED |
+			PS_Y_VERT_FILTER_SELECT(set) |
+			PS_Y_HORZ_FILTER_SELECT(set) |
+			PS_UV_VERT_FILTER_SELECT(set) |
+			PS_UV_HORZ_FILTER_SELECT(set));
+	}
+
+	return PS_FILTER_MEDIUM;
+}
+
+void skl_scaler_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
+			     int id, int set, enum drm_scaling_filter filter)
+{
+	switch (filter) {
+	case DRM_SCALING_FILTER_DEFAULT:
+		break;
+	case DRM_SCALING_FILTER_NEAREST_NEIGHBOR:
+		cnl_program_nearest_filter_coefs(dev_priv, pipe, id, set);
+		break;
+	default:
+		MISSING_CASE(filter);
+	}
+}
+
 static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index adb1225a3480..c22308a042d8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -28,6 +28,7 @@
 #include <drm/drm_util.h>
 
 enum link_m_n_set;
+enum drm_scaling_filter;
 struct dpll;
 struct drm_connector;
 struct drm_device;
@@ -587,6 +588,9 @@ void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
 u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center);
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
 void skl_scaler_disable(const struct intel_crtc_state *old_crtc_state);
+u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set);
+void skl_scaler_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
+			     int id, int set, enum drm_scaling_filter filter);
 void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state);
 u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
 			const struct intel_plane_state *plane_state);
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 5/5] drm/i915: Enable scaling filter for plane and CRTC

[Pankaj Bharadiya]

GEN >= 10 hardware supports the programmable scaler filter.

Attach scaling filter property for CRTC and plane for GEN >= 10
hardwares and program scaler filter based on the selected filter
type.

changes since v2:
* Use updated functions
* Add ps_ctrl var to contain the full PS_CTRL register value (Ville)
* Duplicate the scaling filter in crtc and plane hw state (Ville)
changes since v1:
* None
Changes since RFC:
* Enable properties for GEN >= 10 platforms (Ville)
* Do not round off the crtc co-ordinate (Danial Stone, Ville)
* Add new functions to handle scaling filter setup (Ville)
* Remove coefficient set 0 hardcoding.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.c   |  1 +
 drivers/gpu/drm/i915/display/intel_display.c    | 17 +++++++++++++++--
 .../gpu/drm/i915/display/intel_display_types.h  |  2 ++
 drivers/gpu/drm/i915/display/intel_sprite.c     | 15 +++++++++++++--
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 25dfeb3197aa..cfcc8df75656 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -262,6 +262,7 @@ void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
 	plane_state->hw.rotation = from_plane_state->uapi.rotation;
 	plane_state->hw.color_encoding = from_plane_state->uapi.color_encoding;
 	plane_state->hw.color_range = from_plane_state->uapi.color_range;
+	plane_state->hw.scaling_filter = from_plane_state->uapi.scaling_filter;
 }
 
 void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 85c29a69519f..3fd52aebdc2e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6349,6 +6349,7 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 		int pfit_w, pfit_h, hscale, vscale;
 		unsigned long irqflags;
 		int id;
+		u32 ps_ctrl;
 
 		if (drm_WARN_ON(&dev_priv->drm,
 				crtc_state->scaler_state.scaler_id < 0))
@@ -6365,10 +6366,15 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 
 		id = scaler_state->scaler_id;
 
+		ps_ctrl = skl_scaler_get_filter_select(crtc_state->hw.scaling_filter, 0);
+		ps_ctrl |=  PS_SCALER_EN | scaler_state->scalers[id].mode;
+
 		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
-				  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
+		skl_scaler_setup_filter(dev_priv, pipe, id, 0,
+					crtc_state->hw.scaling_filter);
+
+		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), ps_ctrl);
 		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
 				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
 		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
@@ -13318,6 +13324,7 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
 	crtc_state->hw.active = crtc_state->uapi.active;
 	crtc_state->hw.mode = crtc_state->uapi.mode;
 	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
+	crtc_state->hw.scaling_filter = crtc_state->uapi.scaling_filter;
 	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
 }
 
@@ -13328,6 +13335,7 @@ static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state
 	WARN_ON(drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
 
 	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
+	crtc_state->uapi.scaling_filter = crtc_state->hw.scaling_filter;
 
 	/* copy color blobs to uapi */
 	drm_property_replace_blob(&crtc_state->uapi.degamma_lut,
@@ -16802,6 +16810,11 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc;
 	}
 
+	if (INTEL_GEN(dev_priv) >= 10)
+		drm_crtc_create_scaling_filter_property(&crtc->base,
+						BIT(DRM_SCALING_FILTER_DEFAULT) |
+						BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
+
 	intel_color_init(crtc);
 
 	intel_crtc_crc_init(crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 7de4249f2292..8b1edd34fbd8 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -522,6 +522,7 @@ struct intel_plane_state {
 		unsigned int rotation;
 		enum drm_color_encoding color_encoding;
 		enum drm_color_range color_range;
+		enum drm_scaling_filter scaling_filter;
 	} hw;
 
 	struct i915_ggtt_view view;
@@ -801,6 +802,7 @@ struct intel_crtc_state {
 		bool active, enable;
 		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
 		struct drm_display_mode mode, adjusted_mode;
+		enum drm_scaling_filter scaling_filter;
 	} hw;
 
 	/**
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index deda351719db..c589d5953cd5 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -413,6 +413,7 @@ skl_program_scaler(struct intel_plane *plane,
 	u16 y_hphase, uv_rgb_hphase;
 	u16 y_vphase, uv_rgb_vphase;
 	int hscale, vscale;
+	u32 ps_ctrl;
 
 	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
 				      &plane_state->uapi.dst,
@@ -439,8 +440,13 @@ skl_program_scaler(struct intel_plane *plane,
 		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
 	}
 
-	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
-			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode);
+	ps_ctrl = skl_scaler_get_filter_select(plane_state->hw.scaling_filter, 0);
+	ps_ctrl |= PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode;
+
+	skl_scaler_setup_filter(dev_priv, pipe, scaler_id, 0,
+				plane_state->hw.scaling_filter);
+
+	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
 	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id),
 			  PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
 	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id),
@@ -3121,6 +3127,11 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 
 	drm_plane_create_zpos_immutable_property(&plane->base, plane_id);
 
+	if (INTEL_GEN(dev_priv) >= 10)
+		drm_plane_create_scaling_filter_property(&plane->base,
+						BIT(DRM_SCALING_FILTER_DEFAULT) |
+						BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
+
 	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
 
 	return plane;
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/5] Introduce drm scaling filter property

[Simon Ser]

Hi,

On Monday, March 30, 2020 8:38 PM, Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com> wrote:

> Userspace patch series link: https://github.com/lrusak/xbmc/pull/24

This pull request is against a fork, not the official Kodi repository.
Are there any plans to upstream the change so that users can benefit
from it? Is there a reason why this pull request hasn't been opened
against the official repo?

Thanks,

Simon


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/5] drm/drm-kms.rst: Add plane and CRTC scaling filter property documentation

[Simon Ser]

On Monday, March 30, 2020 8:38 PM, Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com> wrote:

> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 906771e03103..b0335e9d887c 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -509,6 +509,18 @@ Variable Refresh Properties
> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :doc: Variable refresh properties
>
> +Plane Scaling Filter Property
> +-----------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
>
> -   :doc: Plane scaling filter property
> -
>
> +CRTC Scaling Filter Property
> +-----------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
>
> -   :doc: CRTC scaling filter property
> -
>

This adds whole new sections just for the scaling filter property.
Shouldn't we use the existing "Plane properties" section defined in
drm_blend.c for plane props? (Same for CRTC props.)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/5] Introduce drm scaling filter property

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 714 bytes --]

On Mon, 30 Mar 2020 19:30:27 +0000
Simon Ser <contact@emersion.fr> wrote:

> Hi,
> 
> On Monday, March 30, 2020 8:38 PM, Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> 
> > Userspace patch series link: https://github.com/lrusak/xbmc/pull/24  
> 
> This pull request is against a fork, not the official Kodi repository.
> Are there any plans to upstream the change so that users can benefit
> from it? Is there a reason why this pull request hasn't been opened
> against the official repo?

That is an excellent observation. Merge requests against forks do not
count:
https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.

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

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
> index d6ad60ab0d38..a6fc95942851 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1221,3 +1221,81 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  
>  	return ret;
>  }
> +
> +/**
> + * DOC: Plane scaling filter property
> + *
> + * SCALING_FILTER:
> + *
> + *	Indicates scaling filter to be used for plane 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 plane by calling
> + * drm_plane_create_scaling_filter_property
> + */
> +
> +/**
> + * 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_device *dev = plane->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(&plane->base, prop,
> +				   DRM_SCALING_FILTER_DEFAULT);
> +	plane->scaling_filter_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 59b51a09cae6..ba839e5e357d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -324,6 +324,13 @@ struct drm_crtc_state {
>  	 */
>  	bool self_refresh_active;
>  
> +	/**
> +	 * @scaling_filter:
> +	 *
> +	 * Scaling filter to be applied
> +	 */
> +	enum drm_scaling_filter scaling_filter;
> +
>  	/**
>  	 * @event:
>  	 *
> @@ -1083,6 +1090,12 @@ struct drm_crtc {
>  	/** @properties: property tracking for this CRTC */
>  	struct drm_object_properties properties;
>  
> +	/**
> +	 * @scaling_filter_property: property to apply a particular filter while
> +	 * scaling.
> +	 */
> +	struct drm_property *scaling_filter_property;
> +
>  	/**
>  	 * @state:
>  	 *
> @@ -1266,4 +1279,7 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
>  #define drm_for_each_crtc(crtc, dev) \
>  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
>  
> +int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
> +					    unsigned int supported_filters);
> +
>  #endif /* __DRM_CRTC_H__ */
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 3f396d94afe4..1d82b264e5e4 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -35,6 +35,11 @@ struct drm_crtc;
>  struct drm_printer;
>  struct drm_modeset_acquire_ctx;
>  
> +enum drm_scaling_filter {
> +	DRM_SCALING_FILTER_DEFAULT,
> +	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
> +};
> +
>  /**
>   * struct drm_plane_state - mutable plane state
>   *
> @@ -214,6 +219,13 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +	/**
> +	 * @scaling_filter:
> +	 *
> +	 * Scaling filter to be applied
> +	 */
> +	enum drm_scaling_filter scaling_filter;
> +
>  	/**
>  	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
>  	 * and for async plane updates.
> @@ -724,6 +736,12 @@ struct drm_plane {
>  	 * See drm_plane_create_color_properties().
>  	 */
>  	struct drm_property *color_range_property;
> +
> +	/**
> +	 * @scaling_filter_property: property to apply a particular filter while
> +	 * scaling.
> +	 */
> +	struct drm_property *scaling_filter_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> @@ -862,4 +880,7 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state)
>  					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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Bharadiya,Pankaj]

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);
 /* 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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