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

[PATCH v2 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 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                   |  33 ++++++
 drivers/gpu/drm/drm_mode_config.c            |  26 +++++
 drivers/gpu/drm/drm_plane.c                  |  33 ++++++
 drivers/gpu/drm/i915/display/intel_display.c | 104 ++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_display.h |   2 +
 drivers/gpu/drm/i915/display/intel_sprite.c  |  31 +++++-
 drivers/gpu/drm/i915/i915_reg.h              |  48 +++++++++
 include/drm/drm_crtc.h                       |  13 +++
 include/drm/drm_mode_config.h                |  12 +++
 include/drm/drm_plane.h                      |  13 +++
 12 files changed, 332 insertions(+), 3 deletions(-)

-- 
2.23.0

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

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

[Pankaj Bharadiya]

Introduce new plane and 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_enable_scaling_filter() and for a CRTC by calling
drm_crtc_enable_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 v1:
* None
changes since RFC:
* Add separate properties for plane and CRTC (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/drm_atomic_uapi.c |  8 ++++++++
 drivers/gpu/drm/drm_crtc.c        | 33 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_mode_config.c | 26 ++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c       | 33 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h            | 13 ++++++++++++
 include/drm/drm_mode_config.h     | 12 +++++++++++
 include/drm/drm_plane.h           | 13 ++++++++++++
 7 files changed, 138 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..3c72ab52ff62 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		return ret;
 	} else if (property == config->prop_vrr_enabled) {
 		state->vrr_enabled = val;
+	} else if (property == config->crtc_scaling_filter_property) {
+		state->scaling_filter = val;
 	} else if (property == config->degamma_lut_property) {
 		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
@@ -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 == config->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 == config->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 == config->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..c8d387891dd5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -748,3 +748,36 @@ 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_enable_scaling_filter()
+ */
+
+/**
+ * drm_crtc_enable_scaling_filter - Enables CRTC scaling filter property.
+ * @crtc: CRTC on which to enable scaling filter property.
+ *
+ * This function lets driver to enable the scaling filter property on a CRTC.
+ */
+void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+
+	drm_object_attach_property(&crtc->base,
+				   dev->mode_config.crtc_scaling_filter_property,
+				   0);
+}
+EXPORT_SYMBOL(drm_crtc_enable_scaling_filter);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index e1ec1bb7068d..483705581930 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -214,6 +214,16 @@ static const struct drm_prop_enum_list drm_plane_type_enum_list[] = {
 	{ DRM_PLANE_TYPE_CURSOR, "Cursor" },
 };
 
+static const struct drm_prop_enum_list drm_crtc_scaling_filter_enum_list[] = {
+	{ DRM_CRTC_SCALING_FILTER_DEFAULT, "Default" },
+	{ DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
+};
+
+static const struct drm_prop_enum_list drm_plane_scaling_filter_enum_list[] = {
+	{ DRM_PLANE_SCALING_FILTER_DEFAULT, "Default" },
+	{ DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
+};
+
 static int drm_mode_create_standard_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
@@ -370,6 +380,22 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.modifiers_property = prop;
 
+	prop = drm_property_create_enum(dev, 0,
+				"SCALING_FILTER",
+				drm_crtc_scaling_filter_enum_list,
+				ARRAY_SIZE(drm_crtc_scaling_filter_enum_list));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.crtc_scaling_filter_property = prop;
+
+	prop = drm_property_create_enum(dev, 0,
+				"SCALING_FILTER",
+				drm_plane_scaling_filter_enum_list,
+				ARRAY_SIZE(drm_plane_scaling_filter_enum_list));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.plane_scaling_filter_property = prop;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..f71cf50a4ef9 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1221,3 +1221,36 @@ 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_enable_scaling_filter()
+ */
+
+/**
+ * drm_plane_enable_scaling_filter - Enables plane scaling filter property.
+ * @plane: Plane on which to enable scaling filter property.
+ *
+ * This function lets driver to enable the scaling filter property on a plane.
+ */
+void drm_plane_enable_scaling_filter(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+
+	drm_object_attach_property(&plane->base,
+				   dev->mode_config.plane_scaling_filter_property,
+				   0);
+}
+EXPORT_SYMBOL(drm_plane_enable_scaling_filter);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 59b51a09cae6..3187df6874d4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -76,6 +76,10 @@ struct drm_atomic_state;
 struct drm_crtc_helper_funcs;
 struct drm_plane_helper_funcs;
 
+enum drm_crtc_scaling_filter {
+	DRM_CRTC_SCALING_FILTER_DEFAULT,
+	DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR,
+};
 /**
  * struct drm_crtc_state - mutable CRTC state
  *
@@ -296,6 +300,13 @@ struct drm_crtc_state {
 	 */
 	u32 target_vblank;
 
+	/**
+	 * @scaling_filter:
+	 *
+	 * Scaling filter mode to be applied
+	 */
+	enum drm_crtc_scaling_filter scaling_filter;
+
 	/**
 	 * @async_flip:
 	 *
@@ -1266,4 +1277,6 @@ 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)
 
+void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc);
+
 #endif /* __DRM_CRTC_H__ */
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30339f0..2b394561359b 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -914,6 +914,18 @@ struct drm_mode_config {
 	 */
 	struct drm_property *modifiers_property;
 
+	/**
+	 * @crtc_scaling_filter_property: CRTC property to apply a particular
+	 * filter while scaling.
+	 */
+	struct drm_property *crtc_scaling_filter_property;
+
+	/**
+	 * @plane_scaling_filter_property: Plane property to apply a particular
+	 * filter while scaling.
+	 */
+	struct drm_property *plane_scaling_filter_property;
+
 	/* cursor size */
 	uint32_t cursor_width, cursor_height;
 
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3f396d94afe4..f75cee8c4ffa 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -35,6 +35,10 @@ struct drm_crtc;
 struct drm_printer;
 struct drm_modeset_acquire_ctx;
 
+enum drm_plane_scaling_filter {
+	DRM_PLANE_SCALING_FILTER_DEFAULT,
+	DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR,
+};
 /**
  * struct drm_plane_state - mutable plane state
  *
@@ -214,6 +218,13 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+	/**
+	 * @scaling_filter:
+	 *
+	 * Scaling filter mode to be applied
+	 */
+	enum drm_plane_scaling_filter scaling_filter;
+
 	/**
 	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
 	 * and for async plane updates.
@@ -862,4 +873,6 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state)
 					state->fb_damage_clips->data : NULL);
 }
 
+void drm_plane_enable_scaling_filter(struct drm_plane *plane);
+
 #endif
-- 
2.23.0

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

[PATCH v2 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 v2 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 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 | 48 +++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9c53fe918be6..d40f12d2a6b5 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,25 @@ 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_COEF_SET1_INDEX_1A	   0x681A0
+#define _PS_COEF_SET1_INDEX_2A	   0x682A0
+#define _PS_COEF_SET1_INDEX_1B	   0x689A0
+#define _PS_COEF_SET1_INDEX_2B	   0x68AA0
+#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 _PS_COEF_SET1_DATA_1A	   0x681A4
+#define _PS_COEF_SET1_DATA_2A	   0x682A4
+#define _PS_COEF_SET1_DATA_1B	   0x689A4
+#define _PS_COEF_SET1_DATA_2B	   0x68AA4
+
 #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),       \
@@ -7310,6 +7334,30 @@ enum {
 			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
 			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))
 
+#define _SKL_PS_COEF_INDEX_SET0(pipe, id)  _ID(pipe,    \
+			_ID(id, _PS_COEF_SET0_INDEX_1A, _PS_COEF_SET0_INDEX_2A), \
+			_ID(id, _PS_COEF_SET0_INDEX_1B, _PS_COEF_SET0_INDEX_2B))
+
+#define _SKL_PS_COEF_INDEX_SET1(pipe, id)  _ID(pipe,    \
+			_ID(id, _PS_COEF_SET1_INDEX_1A, _PS_COEF_SET1_INDEX_2A), \
+			_ID(id, _PS_COEF_SET1_INDEX_1B, _PS_COEF_SET1_INDEX_2B))
+
+#define _SKL_PS_COEF_DATA_SET0(pipe, id)  _ID(pipe,     \
+			_ID(id, _PS_COEF_SET0_DATA_1A, _PS_COEF_SET0_DATA_2A), \
+			_ID(id, _PS_COEF_SET0_DATA_1B, _PS_COEF_SET0_DATA_2B))
+
+#define _SKL_PS_COEF_DATA_SET1(pipe, id)  _ID(pipe,     \
+			_ID(id, _PS_COEF_SET1_DATA_1A, _PS_COEF_SET1_DATA_2A), \
+			_ID(id, _PS_COEF_SET1_DATA_1B, _PS_COEF_SET1_DATA_2B))
+
+#define SKL_PS_COEF_INDEX_SET(pipe, id, set) \
+			_MMIO_PIPE(set, _SKL_PS_COEF_INDEX_SET0(pipe, id), \
+			    _SKL_PS_COEF_INDEX_SET1(pipe, id))
+
+#define SKL_PS_COEF_DATA_SET(pipe, id, set) \
+			_MMIO_PIPE(set, _SKL_PS_COEF_DATA_SET0(pipe, id), \
+			    _SKL_PS_COEF_DATA_SET1(pipe, id))
+
 /* 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 v2 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 skl_scaler_setup_nearest_neighbor_filter() function which
configures the scaler filter coefficients to enable nearest-neighbor
filtering.

Bspec: 49247

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 | 72 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  2 +
 2 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 8f23c4d51c33..791dd908aa89 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6237,6 +6237,78 @@ void skl_scaler_disable(const struct intel_crtc_state *old_crtc_state)
 		skl_detach_scaler(crtc, i);
 }
 
+static int skl_coef_tap(int i)
+{
+	return i % 7;
+}
+
+static u16 skl_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
+ *
+ */
+
+void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
+					      enum pipe pipe, int id, int set)
+{
+	int i;
+
+	/*enable the index auto increment.*/
+	intel_de_write_fw(dev_priv,
+			  SKL_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 = skl_coef_tap(i);
+		tmp = skl_nearest_filter_coef(t);
+
+		t = skl_coef_tap(i+1);
+		tmp |= skl_nearest_filter_coef(t)<<16;
+
+		intel_de_write_fw(dev_priv, SKL_PS_COEF_DATA_SET(pipe, id, set),
+				  tmp);
+	}
+}
+
 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..88f3c77f6806 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -587,6 +587,8 @@ 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);
+void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
+					      enum pipe pipe, int id, int set);
 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 v2 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 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>
---
 drivers/gpu/drm/i915/display/intel_display.c | 32 ++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_sprite.c  | 31 ++++++++++++++++++-
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 791dd908aa89..4b3387ee332e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6309,6 +6309,25 @@ void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
 	}
 }
 
+static u32
+skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
+			  int id, int set, enum drm_crtc_scaling_filter filter)
+{
+	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
+
+	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
+		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
+							 set);
+		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
+				PS_UV_VERT_FILTER_SELECT(set) |
+				PS_UV_HORZ_FILTER_SELECT(set) |
+				PS_Y_VERT_FILTER_SELECT(set) |
+				PS_Y_HORZ_FILTER_SELECT(set);
+
+	}
+	return scaler_filter_ctl;
+}
+
 static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 	enum pipe pipe = crtc->pipe;
 	const struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
+	const struct drm_crtc_state *state = &crtc_state->uapi;
 
 	if (crtc_state->pch_pfit.enabled) {
 		u16 uv_rgb_hphase, uv_rgb_vphase;
 		int pfit_w, pfit_h, hscale, vscale;
 		unsigned long irqflags;
 		int id;
+		int scaler_filter_ctl;
 
 		if (drm_WARN_ON(&dev_priv->drm,
 				crtc_state->scaler_state.scaler_id < 0))
@@ -6340,8 +6361,12 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 
 		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);
+		scaler_filter_ctl =
+			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
+						state->scaling_filter);
+		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
+				  PS_SCALER_EN | scaler_filter_ctl |
+				  scaler_state->scalers[id].mode);
 		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),
@@ -16777,6 +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
+
 	intel_color_init(crtc);
 
 	intel_crtc_crc_init(crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index deda351719db..ac3fd9843ace 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
 		return min(8192 * cpp, 32768);
 }
 
+static u32
+skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
+			      int id, int set,
+			      enum drm_plane_scaling_filter filter)
+{
+	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
+
+	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
+		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
+							 set);
+		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
+				PS_UV_VERT_FILTER_SELECT(set) |
+				PS_UV_HORZ_FILTER_SELECT(set) |
+				PS_Y_VERT_FILTER_SELECT(set) |
+				PS_Y_HORZ_FILTER_SELECT(set);
+
+	}
+	return scaler_filter_ctl;
+}
+
 static void
 skl_program_scaler(struct intel_plane *plane,
 		   const struct intel_crtc_state *crtc_state,
@@ -406,6 +426,7 @@ skl_program_scaler(struct intel_plane *plane,
 	int scaler_id = plane_state->scaler_id;
 	const struct intel_scaler *scaler =
 		&crtc_state->scaler_state.scalers[scaler_id];
+	const struct drm_plane_state *state = &plane_state->uapi;
 	int crtc_x = plane_state->uapi.dst.x1;
 	int crtc_y = plane_state->uapi.dst.y1;
 	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
@@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
 	u16 y_hphase, uv_rgb_hphase;
 	u16 y_vphase, uv_rgb_vphase;
 	int hscale, vscale;
+	int scaler_filter_ctl;
 
 	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
 				      &plane_state->uapi.dst,
@@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
 		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
 	}
 
+	scaler_filter_ctl =
+		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
+					      state->scaling_filter);
 	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
-			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode);
+			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
+			  scaler->mode | scaler_filter_ctl);
 	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
+
 	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 v2 1/5] drm: Introduce plane and CRTC scaling filter properties

[Ville Syrjälä]

On Thu, Mar 19, 2020 at 03:50:59PM +0530, Pankaj Bharadiya wrote:
> Introduce new plane and 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_enable_scaling_filter() and for a CRTC by calling
> drm_crtc_enable_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 v1:
> * None
> changes since RFC:
> * Add separate properties for plane and CRTC (Ville)

I actually meant the prop should be per-object, not just separate
prop for planes and crtcs.

> 
> 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/drm_atomic_uapi.c |  8 ++++++++
>  drivers/gpu/drm/drm_crtc.c        | 33 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_mode_config.c | 26 ++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c       | 33 +++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h            | 13 ++++++++++++
>  include/drm/drm_mode_config.h     | 12 +++++++++++
>  include/drm/drm_plane.h           | 13 ++++++++++++
>  7 files changed, 138 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index a1e5e262bae2..3c72ab52ff62 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		return ret;
>  	} else if (property == config->prop_vrr_enabled) {
>  		state->vrr_enabled = val;
> +	} else if (property == config->crtc_scaling_filter_property) {
> +		state->scaling_filter = val;
>  	} else if (property == config->degamma_lut_property) {
>  		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->degamma_lut,
> @@ -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 == config->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 == config->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 == config->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..c8d387891dd5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -748,3 +748,36 @@ 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_enable_scaling_filter()
> + */
> +
> +/**
> + * drm_crtc_enable_scaling_filter - Enables CRTC scaling filter property.
> + * @crtc: CRTC on which to enable scaling filter property.
> + *
> + * This function lets driver to enable the scaling filter property on a CRTC.
> + */
> +void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +
> +	drm_object_attach_property(&crtc->base,
> +				   dev->mode_config.crtc_scaling_filter_property,
> +				   0);
> +}
> +EXPORT_SYMBOL(drm_crtc_enable_scaling_filter);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index e1ec1bb7068d..483705581930 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -214,6 +214,16 @@ static const struct drm_prop_enum_list drm_plane_type_enum_list[] = {
>  	{ DRM_PLANE_TYPE_CURSOR, "Cursor" },
>  };
>  
> +static const struct drm_prop_enum_list drm_crtc_scaling_filter_enum_list[] = {
> +	{ DRM_CRTC_SCALING_FILTER_DEFAULT, "Default" },
> +	{ DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
> +};
> +
> +static const struct drm_prop_enum_list drm_plane_scaling_filter_enum_list[] = {
> +	{ DRM_PLANE_SCALING_FILTER_DEFAULT, "Default" },
> +	{ DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest Neighbor" },
> +};
> +
>  static int drm_mode_create_standard_properties(struct drm_device *dev)
>  {
>  	struct drm_property *prop;
> @@ -370,6 +380,22 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.modifiers_property = prop;
>  
> +	prop = drm_property_create_enum(dev, 0,
> +				"SCALING_FILTER",
> +				drm_crtc_scaling_filter_enum_list,
> +				ARRAY_SIZE(drm_crtc_scaling_filter_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.crtc_scaling_filter_property = prop;
> +
> +	prop = drm_property_create_enum(dev, 0,
> +				"SCALING_FILTER",
> +				drm_plane_scaling_filter_enum_list,
> +				ARRAY_SIZE(drm_plane_scaling_filter_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.plane_scaling_filter_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f71cf50a4ef9 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1221,3 +1221,36 @@ 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_enable_scaling_filter()
> + */
> +
> +/**
> + * drm_plane_enable_scaling_filter - Enables plane scaling filter property.
> + * @plane: Plane on which to enable scaling filter property.
> + *
> + * This function lets driver to enable the scaling filter property on a plane.
> + */
> +void drm_plane_enable_scaling_filter(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +
> +	drm_object_attach_property(&plane->base,
> +				   dev->mode_config.plane_scaling_filter_property,
> +				   0);
> +}
> +EXPORT_SYMBOL(drm_plane_enable_scaling_filter);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 59b51a09cae6..3187df6874d4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -76,6 +76,10 @@ struct drm_atomic_state;
>  struct drm_crtc_helper_funcs;
>  struct drm_plane_helper_funcs;
>  
> +enum drm_crtc_scaling_filter {
> +	DRM_CRTC_SCALING_FILTER_DEFAULT,
> +	DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR,
> +};
>  /**
>   * struct drm_crtc_state - mutable CRTC state
>   *
> @@ -296,6 +300,13 @@ struct drm_crtc_state {
>  	 */
>  	u32 target_vblank;
>  
> +	/**
> +	 * @scaling_filter:
> +	 *
> +	 * Scaling filter mode to be applied
> +	 */
> +	enum drm_crtc_scaling_filter scaling_filter;
> +
>  	/**
>  	 * @async_flip:
>  	 *
> @@ -1266,4 +1277,6 @@ 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)
>  
> +void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc);
> +
>  #endif /* __DRM_CRTC_H__ */
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 3bcbe30339f0..2b394561359b 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -914,6 +914,18 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *modifiers_property;
>  
> +	/**
> +	 * @crtc_scaling_filter_property: CRTC property to apply a particular
> +	 * filter while scaling.
> +	 */
> +	struct drm_property *crtc_scaling_filter_property;
> +
> +	/**
> +	 * @plane_scaling_filter_property: Plane property to apply a particular
> +	 * filter while scaling.
> +	 */
> +	struct drm_property *plane_scaling_filter_property;
> +
>  	/* cursor size */
>  	uint32_t cursor_width, cursor_height;
>  
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 3f396d94afe4..f75cee8c4ffa 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -35,6 +35,10 @@ struct drm_crtc;
>  struct drm_printer;
>  struct drm_modeset_acquire_ctx;
>  
> +enum drm_plane_scaling_filter {
> +	DRM_PLANE_SCALING_FILTER_DEFAULT,
> +	DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR,
> +};
>  /**
>   * struct drm_plane_state - mutable plane state
>   *
> @@ -214,6 +218,13 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +	/**
> +	 * @scaling_filter:
> +	 *
> +	 * Scaling filter mode to be applied
> +	 */
> +	enum drm_plane_scaling_filter scaling_filter;
> +
>  	/**
>  	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
>  	 * and for async plane updates.
> @@ -862,4 +873,6 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state)
>  					state->fb_damage_clips->data : NULL);
>  }
>  
> +void drm_plane_enable_scaling_filter(struct drm_plane *plane);
> +
>  #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 v2 3/5] drm/i915: Introduce scaling filter related registers and bit fields.

[Ville Syrjälä]

On Thu, Mar 19, 2020 at 03:51:01PM +0530, Pankaj Bharadiya wrote:
> Introduce scaler registers and bit fields needed to configure the
> scaling filter in prgrammed mode and configure scaling filter
> coefficients.
> 
> 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 | 48 +++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c53fe918be6..d40f12d2a6b5 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,25 @@ 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_COEF_SET1_INDEX_1A	   0x681A0
> +#define _PS_COEF_SET1_INDEX_2A	   0x682A0
> +#define _PS_COEF_SET1_INDEX_1B	   0x689A0
> +#define _PS_COEF_SET1_INDEX_2B	   0x68AA0
> +#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 _PS_COEF_SET1_DATA_1A	   0x681A4
> +#define _PS_COEF_SET1_DATA_2A	   0x682A4
> +#define _PS_COEF_SET1_DATA_1B	   0x689A4
> +#define _PS_COEF_SET1_DATA_2B	   0x68AA4
> +
>  #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),       \
> @@ -7310,6 +7334,30 @@ enum {
>  			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
>  			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))
>  
> +#define _SKL_PS_COEF_INDEX_SET0(pipe, id)  _ID(pipe,    \
> +			_ID(id, _PS_COEF_SET0_INDEX_1A, _PS_COEF_SET0_INDEX_2A), \
> +			_ID(id, _PS_COEF_SET0_INDEX_1B, _PS_COEF_SET0_INDEX_2B))
> +
> +#define _SKL_PS_COEF_INDEX_SET1(pipe, id)  _ID(pipe,    \
> +			_ID(id, _PS_COEF_SET1_INDEX_1A, _PS_COEF_SET1_INDEX_2A), \
> +			_ID(id, _PS_COEF_SET1_INDEX_1B, _PS_COEF_SET1_INDEX_2B))
> +
> +#define _SKL_PS_COEF_DATA_SET0(pipe, id)  _ID(pipe,     \
> +			_ID(id, _PS_COEF_SET0_DATA_1A, _PS_COEF_SET0_DATA_2A), \
> +			_ID(id, _PS_COEF_SET0_DATA_1B, _PS_COEF_SET0_DATA_2B))
> +
> +#define _SKL_PS_COEF_DATA_SET1(pipe, id)  _ID(pipe,     \
> +			_ID(id, _PS_COEF_SET1_DATA_1A, _PS_COEF_SET1_DATA_2A), \
> +			_ID(id, _PS_COEF_SET1_DATA_1B, _PS_COEF_SET1_DATA_2B))
> +
> +#define SKL_PS_COEF_INDEX_SET(pipe, id, set) \
> +			_MMIO_PIPE(set, _SKL_PS_COEF_INDEX_SET0(pipe, id), \
> +			    _SKL_PS_COEF_INDEX_SET1(pipe, id))
> +
> +#define SKL_PS_COEF_DATA_SET(pipe, id, set) \
> +			_MMIO_PIPE(set, _SKL_PS_COEF_DATA_SET0(pipe, id), \
> +			    _SKL_PS_COEF_DATA_SET1(pipe, id))

I'd name those CNL_PS_COEF_{DATA,INDEX}(). Or maybe eeven GLK_ since it
already has this despite not being officially supported.

Also I'd probably just have used +(set)*8 instead of adding another trip
through _PICK_EVEN(). It's getting a bit hard to read this.

> +
>  /* legacy palette */
>  #define _LGC_PALETTE_A           0x4a000
>  #define _LGC_PALETTE_B           0x4a800
> -- 
> 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 v2 4/5] drm/i915/display: Add Nearest-neighbor based integer scaling support

[Ville Syrjälä]

On Thu, Mar 19, 2020 at 03:51:02PM +0530, Pankaj Bharadiya wrote:
> 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 skl_scaler_setup_nearest_neighbor_filter() function which
> configures the scaler filter coefficients to enable nearest-neighbor
> filtering.
> 
> Bspec: 49247
> 
> 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 | 72 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  2 +
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 8f23c4d51c33..791dd908aa89 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6237,6 +6237,78 @@ void skl_scaler_disable(const struct intel_crtc_state *old_crtc_state)
>  		skl_detach_scaler(crtc, i);
>  }
>  
> +static int skl_coef_tap(int i)
> +{
> +	return i % 7;
> +}
> +
> +static u16 skl_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
> + *
> + */
> +
> +void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> +					      enum pipe pipe, int id, int set)
> +{
> +	int i;
> +
> +	/*enable the index auto increment.*/

Pointless comment, and also misformatted.

> +	intel_de_write_fw(dev_priv,
> +			  SKL_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 = skl_coef_tap(i);
> +		tmp = skl_nearest_filter_coef(t);
> +
> +		t = skl_coef_tap(i+1);
> +		tmp |= skl_nearest_filter_coef(t)<<16;

Missing spaces.

> +
> +		intel_de_write_fw(dev_priv, SKL_PS_COEF_DATA_SET(pipe, id, set),
> +				  tmp);
> +	}

I'd maybe reset the index back to 0 here and disable the auto-increment
bit.

> +}
> +
>  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..88f3c77f6806 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -587,6 +587,8 @@ 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);
> +void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> +					      enum pipe pipe, int id, int set);
>  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

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

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

[Ville Syrjälä]

On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> 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 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>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 32 ++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_sprite.c  | 31 ++++++++++++++++++-
>  2 files changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 791dd908aa89..4b3387ee332e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6309,6 +6309,25 @@ void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static u32
> +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
> +			  int id, int set, enum drm_crtc_scaling_filter filter)
> +{
> +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> +
> +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> +							 set);
> +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> +				PS_UV_VERT_FILTER_SELECT(set) |
> +				PS_UV_HORZ_FILTER_SELECT(set) |
> +				PS_Y_VERT_FILTER_SELECT(set) |
> +				PS_Y_HORZ_FILTER_SELECT(set);
> +
> +	}
> +	return scaler_filter_ctl;

This function does too many things.

> +}
> +
>  static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  	enum pipe pipe = crtc->pipe;
>  	const struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> +	const struct drm_crtc_state *state = &crtc_state->uapi;

Pls don't add this kind of aliases. We're moving away from using the
drm_ types as much as possible.

>  
>  	if (crtc_state->pch_pfit.enabled) {
>  		u16 uv_rgb_hphase, uv_rgb_vphase;
>  		int pfit_w, pfit_h, hscale, vscale;
>  		unsigned long irqflags;
>  		int id;
> +		int scaler_filter_ctl;

It's a register value so u32. I'd also 
s/scaler_filter_ctl/filter_select/ or something like that.

Alternatively we could just call it ps_ctrl and have it contain
the full register value for that particular register.

>  
>  		if (drm_WARN_ON(&dev_priv->drm,
>  				crtc_state->scaler_state.scaler_id < 0))
> @@ -6340,8 +6361,12 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  
>  		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);
> +		scaler_filter_ctl =
> +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> +						state->scaling_filter);
> +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> +				  PS_SCALER_EN | scaler_filter_ctl |
> +				  scaler_state->scalers[id].mode);
>  		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),
> @@ -16777,6 +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
> +
>  	intel_color_init(crtc);
>  
>  	intel_crtc_crc_init(crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index deda351719db..ac3fd9843ace 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
>  		return min(8192 * cpp, 32768);
>  }
>  
> +static u32
> +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe,
> +			      int id, int set,
> +			      enum drm_plane_scaling_filter filter)
> +{
> +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> +
> +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> +							 set);
> +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> +				PS_UV_VERT_FILTER_SELECT(set) |
> +				PS_UV_HORZ_FILTER_SELECT(set) |
> +				PS_Y_VERT_FILTER_SELECT(set) |
> +				PS_Y_HORZ_FILTER_SELECT(set);
> +
> +	}
> +	return scaler_filter_ctl;
> +}
> +

We don't want such copy pasta between planes and crtcs.

>  static void
>  skl_program_scaler(struct intel_plane *plane,
>  		   const struct intel_crtc_state *crtc_state,
> @@ -406,6 +426,7 @@ skl_program_scaler(struct intel_plane *plane,
>  	int scaler_id = plane_state->scaler_id;
>  	const struct intel_scaler *scaler =
>  		&crtc_state->scaler_state.scalers[scaler_id];
> +	const struct drm_plane_state *state = &plane_state->uapi;
>  	int crtc_x = plane_state->uapi.dst.x1;
>  	int crtc_y = plane_state->uapi.dst.y1;
>  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
>  	u16 y_hphase, uv_rgb_hphase;
>  	u16 y_vphase, uv_rgb_vphase;
>  	int hscale, vscale;
> +	int scaler_filter_ctl;
>  
>  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
>  				      &plane_state->uapi.dst,
> @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
>  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
>  	}
>  
> +	scaler_filter_ctl =
> +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> +					      state->scaling_filter);
>  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode);
> +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> +			  scaler->mode | scaler_filter_ctl);
>  	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
> +
>  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
>  
>  	return plane;
> -- 
> 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 v2 1/5] drm: Introduce plane and CRTC scaling filter properties

[Laxminarayan Bharadiya, Pankaj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: 23 March 2020 19:52
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> dri-devel@lists.freedesktop.org; daniels@collabora.com; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@linux.ie>
> Subject: Re: [PATCH v2 1/5] drm: Introduce plane and CRTC scaling filter
> properties
> 
> On Thu, Mar 19, 2020 at 03:50:59PM +0530, Pankaj Bharadiya wrote:
> > Introduce new plane and 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_enable_scaling_filter() and for a CRTC by calling
> > drm_crtc_enable_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 v1:
> > * None
> > changes since RFC:
> > * Add separate properties for plane and CRTC (Ville)
> 
> I actually meant the prop should be per-object, not just separate prop for planes
> and crtcs.

My bad, I misunderstood it ☹.
Will add drm_property in drm_crtc and drm_plane.

Thanks,
Pankaj

> 
> >
> > 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/drm_atomic_uapi.c |  8 ++++++++
> >  drivers/gpu/drm/drm_crtc.c        | 33 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_mode_config.c | 26 ++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_plane.c       | 33 +++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h            | 13 ++++++++++++
> >  include/drm/drm_mode_config.h     | 12 +++++++++++
> >  include/drm/drm_plane.h           | 13 ++++++++++++
> >  7 files changed, 138 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index a1e5e262bae2..3c72ab52ff62 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct
> drm_crtc *crtc,
> >  		return ret;
> >  	} else if (property == config->prop_vrr_enabled) {
> >  		state->vrr_enabled = val;
> > +	} else if (property == config->crtc_scaling_filter_property) {
> > +		state->scaling_filter = val;
> >  	} else if (property == config->degamma_lut_property) {
> >  		ret = drm_atomic_replace_property_blob_from_id(dev,
> >  					&state->degamma_lut,
> > @@ -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 == config->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 == config->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 == config->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..c8d387891dd5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -748,3 +748,36 @@ 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_enable_scaling_filter()
> > + */
> > +
> > +/**
> > + * drm_crtc_enable_scaling_filter - Enables CRTC scaling filter property.
> > + * @crtc: CRTC on which to enable scaling filter property.
> > + *
> > + * This function lets driver to enable the scaling filter property on a CRTC.
> > + */
> > +void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc) {
> > +	struct drm_device *dev = crtc->dev;
> > +
> > +	drm_object_attach_property(&crtc->base,
> > +				   dev-
> >mode_config.crtc_scaling_filter_property,
> > +				   0);
> > +}
> > +EXPORT_SYMBOL(drm_crtc_enable_scaling_filter);
> > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > index e1ec1bb7068d..483705581930 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -214,6 +214,16 @@ static const struct drm_prop_enum_list
> drm_plane_type_enum_list[] = {
> >  	{ DRM_PLANE_TYPE_CURSOR, "Cursor" },  };
> >
> > +static const struct drm_prop_enum_list drm_crtc_scaling_filter_enum_list[] =
> {
> > +	{ DRM_CRTC_SCALING_FILTER_DEFAULT, "Default" },
> > +	{ DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest
> Neighbor" }, };
> > +
> > +static const struct drm_prop_enum_list drm_plane_scaling_filter_enum_list[]
> = {
> > +	{ DRM_PLANE_SCALING_FILTER_DEFAULT, "Default" },
> > +	{ DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR, "Nearest
> Neighbor" },
> > +};
> > +
> >  static int drm_mode_create_standard_properties(struct drm_device
> > *dev)  {
> >  	struct drm_property *prop;
> > @@ -370,6 +380,22 @@ static int
> drm_mode_create_standard_properties(struct drm_device *dev)
> >  		return -ENOMEM;
> >  	dev->mode_config.modifiers_property = prop;
> >
> > +	prop = drm_property_create_enum(dev, 0,
> > +				"SCALING_FILTER",
> > +				drm_crtc_scaling_filter_enum_list,
> > +
> 	ARRAY_SIZE(drm_crtc_scaling_filter_enum_list));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.crtc_scaling_filter_property = prop;
> > +
> > +	prop = drm_property_create_enum(dev, 0,
> > +				"SCALING_FILTER",
> > +				drm_plane_scaling_filter_enum_list,
> > +
> 	ARRAY_SIZE(drm_plane_scaling_filter_enum_list));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.plane_scaling_filter_property = prop;
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f71cf50a4ef9 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1221,3 +1221,36 @@ 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_enable_scaling_filter()
> > + */
> > +
> > +/**
> > + * drm_plane_enable_scaling_filter - Enables plane scaling filter property.
> > + * @plane: Plane on which to enable scaling filter property.
> > + *
> > + * This function lets driver to enable the scaling filter property on a plane.
> > + */
> > +void drm_plane_enable_scaling_filter(struct drm_plane *plane) {
> > +	struct drm_device *dev = plane->dev;
> > +
> > +	drm_object_attach_property(&plane->base,
> > +				   dev-
> >mode_config.plane_scaling_filter_property,
> > +				   0);
> > +}
> > +EXPORT_SYMBOL(drm_plane_enable_scaling_filter);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> > 59b51a09cae6..3187df6874d4 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -76,6 +76,10 @@ struct drm_atomic_state;  struct
> > drm_crtc_helper_funcs;  struct drm_plane_helper_funcs;
> >
> > +enum drm_crtc_scaling_filter {
> > +	DRM_CRTC_SCALING_FILTER_DEFAULT,
> > +	DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR,
> > +};
> >  /**
> >   * struct drm_crtc_state - mutable CRTC state
> >   *
> > @@ -296,6 +300,13 @@ struct drm_crtc_state {
> >  	 */
> >  	u32 target_vblank;
> >
> > +	/**
> > +	 * @scaling_filter:
> > +	 *
> > +	 * Scaling filter mode to be applied
> > +	 */
> > +	enum drm_crtc_scaling_filter scaling_filter;
> > +
> >  	/**
> >  	 * @async_flip:
> >  	 *
> > @@ -1266,4 +1277,6 @@ 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)
> >
> > +void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc);
> > +
> >  #endif /* __DRM_CRTC_H__ */
> > diff --git a/include/drm/drm_mode_config.h
> > b/include/drm/drm_mode_config.h index 3bcbe30339f0..2b394561359b
> > 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -914,6 +914,18 @@ struct drm_mode_config {
> >  	 */
> >  	struct drm_property *modifiers_property;
> >
> > +	/**
> > +	 * @crtc_scaling_filter_property: CRTC property to apply a particular
> > +	 * filter while scaling.
> > +	 */
> > +	struct drm_property *crtc_scaling_filter_property;
> > +
> > +	/**
> > +	 * @plane_scaling_filter_property: Plane property to apply a particular
> > +	 * filter while scaling.
> > +	 */
> > +	struct drm_property *plane_scaling_filter_property;
> > +
> >  	/* cursor size */
> >  	uint32_t cursor_width, cursor_height;
> >
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> > 3f396d94afe4..f75cee8c4ffa 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -35,6 +35,10 @@ struct drm_crtc;
> >  struct drm_printer;
> >  struct drm_modeset_acquire_ctx;
> >
> > +enum drm_plane_scaling_filter {
> > +	DRM_PLANE_SCALING_FILTER_DEFAULT,
> > +	DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR,
> > +};
> >  /**
> >   * struct drm_plane_state - mutable plane state
> >   *
> > @@ -214,6 +218,13 @@ struct drm_plane_state {
> >  	 */
> >  	bool visible;
> >
> > +	/**
> > +	 * @scaling_filter:
> > +	 *
> > +	 * Scaling filter mode to be applied
> > +	 */
> > +	enum drm_plane_scaling_filter scaling_filter;
> > +
> >  	/**
> >  	 * @commit: Tracks the pending commit to prevent use-after-free
> conditions,
> >  	 * and for async plane updates.
> > @@ -862,4 +873,6 @@ drm_plane_get_damage_clips(const struct
> drm_plane_state *state)
> >  					state->fb_damage_clips->data : NULL);
> }
> >
> > +void drm_plane_enable_scaling_filter(struct drm_plane *plane);
> > +
> >  #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 v2 3/5] drm/i915: Introduce scaling filter related registers and bit fields.

[Laxminarayan Bharadiya, Pankaj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: 23 March 2020 20:09
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> David Airlie <airlied@linux.ie>
> Subject: Re: [PATCH v2 3/5] drm/i915: Introduce scaling filter related registers
> and bit fields.
> 
> On Thu, Mar 19, 2020 at 03:51:01PM +0530, Pankaj Bharadiya wrote:
> > Introduce scaler registers and bit fields needed to configure the
> > scaling filter in prgrammed mode and configure scaling filter
> > coefficients.
> >
> > 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 | 48
> > +++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 9c53fe918be6..d40f12d2a6b5
> > 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,25 @@ 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_COEF_SET1_INDEX_1A	   0x681A0
> > +#define _PS_COEF_SET1_INDEX_2A	   0x682A0
> > +#define _PS_COEF_SET1_INDEX_1B	   0x689A0
> > +#define _PS_COEF_SET1_INDEX_2B	   0x68AA0
> > +#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 _PS_COEF_SET1_DATA_1A	   0x681A4
> > +#define _PS_COEF_SET1_DATA_2A	   0x682A4
> > +#define _PS_COEF_SET1_DATA_1B	   0x689A4
> > +#define _PS_COEF_SET1_DATA_2B	   0x68AA4
> > +
> >  #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),       \
> > @@ -7310,6 +7334,30 @@ enum {
> >  			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
> >  			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))
> >
> > +#define _SKL_PS_COEF_INDEX_SET0(pipe, id)  _ID(pipe,    \
> > +			_ID(id, _PS_COEF_SET0_INDEX_1A,
> _PS_COEF_SET0_INDEX_2A), \
> > +			_ID(id, _PS_COEF_SET0_INDEX_1B,
> _PS_COEF_SET0_INDEX_2B))
> > +
> > +#define _SKL_PS_COEF_INDEX_SET1(pipe, id)  _ID(pipe,    \
> > +			_ID(id, _PS_COEF_SET1_INDEX_1A,
> _PS_COEF_SET1_INDEX_2A), \
> > +			_ID(id, _PS_COEF_SET1_INDEX_1B,
> _PS_COEF_SET1_INDEX_2B))
> > +
> > +#define _SKL_PS_COEF_DATA_SET0(pipe, id)  _ID(pipe,     \
> > +			_ID(id, _PS_COEF_SET0_DATA_1A,
> _PS_COEF_SET0_DATA_2A), \
> > +			_ID(id, _PS_COEF_SET0_DATA_1B,
> _PS_COEF_SET0_DATA_2B))
> > +
> > +#define _SKL_PS_COEF_DATA_SET1(pipe, id)  _ID(pipe,     \
> > +			_ID(id, _PS_COEF_SET1_DATA_1A,
> _PS_COEF_SET1_DATA_2A), \
> > +			_ID(id, _PS_COEF_SET1_DATA_1B,
> _PS_COEF_SET1_DATA_2B))
> > +
> > +#define SKL_PS_COEF_INDEX_SET(pipe, id, set) \
> > +			_MMIO_PIPE(set, _SKL_PS_COEF_INDEX_SET0(pipe, id),
> \
> > +			    _SKL_PS_COEF_INDEX_SET1(pipe, id))
> > +
> > +#define SKL_PS_COEF_DATA_SET(pipe, id, set) \
> > +			_MMIO_PIPE(set, _SKL_PS_COEF_DATA_SET0(pipe, id),
> \
> > +			    _SKL_PS_COEF_DATA_SET1(pipe, id))
> 
> I'd name those CNL_PS_COEF_{DATA,INDEX}(). Or maybe eeven GLK_ since it
> already has this despite not being officially supported.

All other existing scaler macros start will  SKL_PS_*,  adding new CNL_PS_* may
lead to confusion IMO since we will end up in using mixed SKL_PS_* and CNL_PS_* name
to configure  scaler. 

> 
> Also I'd probably just have used +(set)*8 instead of adding another trip through
> _PICK_EVEN(). It's getting a bit hard to read this.

OK.
How does this sound like?  -

+#define SKL_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 SKL_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)

Thanks,
Pankaj
> 
> > +
> >  /* legacy palette */
> >  #define _LGC_PALETTE_A           0x4a000
> >  #define _LGC_PALETTE_B           0x4a800
> > --
> > 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 v2 5/5] drm/i915: Enable scaling filter for plane and CRTC

[Laxminarayan Bharadiya, Pankaj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: 23 March 2020 20:18
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>;
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose
> <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
> 
> On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> > 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 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>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 32
> > ++++++++++++++++++--  drivers/gpu/drm/i915/display/intel_sprite.c  |
> > 31 ++++++++++++++++++-
> >  2 files changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 791dd908aa89..4b3387ee332e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6309,6 +6309,25 @@ void
> skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >
> > +static u32
> > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> pipe,
> > +			  int id, int set, enum drm_crtc_scaling_filter filter) {
> > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > +
> > +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > +							 set);
> > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > +				PS_UV_VERT_FILTER_SELECT(set) |
> > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > +				PS_Y_VERT_FILTER_SELECT(set) |
> > +				PS_Y_HORZ_FILTER_SELECT(set);
> > +
> > +	}
> > +	return scaler_filter_ctl;
> 
> This function does too many things.

I was thinking to have a common function which configures the filter and also
provides the register bits (ps_ctrl) to select a desired filter so that we need
not have extra condition to figure out filter select register bits where this
function is being called.
How about renaming this function to some better name like  
skl_scaler_set_and_get_filter_select() or something else? 
Or shall I breakdown this function into multiple functions? 

Any suggestions?

> 
> > +}
> > +
> >  static void skl_pfit_enable(const struct intel_crtc_state
> > *crtc_state)  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct
> intel_crtc_state *crtc_state)
> >  	enum pipe pipe = crtc->pipe;
> >  	const struct intel_crtc_scaler_state *scaler_state =
> >  		&crtc_state->scaler_state;
> > +	const struct drm_crtc_state *state = &crtc_state->uapi;
> 
> Pls don't add this kind of aliases. We're moving away from using the drm_ types
> as much as possible.

OK.

> 
> >
> >  	if (crtc_state->pch_pfit.enabled) {
> >  		u16 uv_rgb_hphase, uv_rgb_vphase;
> >  		int pfit_w, pfit_h, hscale, vscale;
> >  		unsigned long irqflags;
> >  		int id;
> > +		int scaler_filter_ctl;
> 
> It's a register value so u32. I'd also

Yes, I missed it. Thanks for pointing out.

> s/scaler_filter_ctl/filter_select/ or something like that.
> 
> Alternatively we could just call it ps_ctrl and have it contain the full register
> value for that particular register.

ps_ctrl sounds better, will use this name.

> 
> >
> >  		if (drm_WARN_ON(&dev_priv->drm,
> >  				crtc_state->scaler_state.scaler_id < 0)) @@ -
> 6340,8 +6361,12 @@
> > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> >
> >  		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);
> > +		scaler_filter_ctl =
> > +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> > +						state->scaling_filter);
> > +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> > +				  PS_SCALER_EN | scaler_filter_ctl |
> > +				  scaler_state->scalers[id].mode);
> >  		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), @@ -
> 16777,6
> > +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
> > +
> >  	intel_color_init(crtc);
> >
> >  	intel_crtc_crc_init(crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index deda351719db..ac3fd9843ace 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
> >  		return min(8192 * cpp, 32768);
> >  }
> >
> > +static u32
> > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> pipe,
> > +			      int id, int set,
> > +			      enum drm_plane_scaling_filter filter) {
> > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > +
> > +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > +							 set);
> > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > +				PS_UV_VERT_FILTER_SELECT(set) |
> > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > +				PS_Y_VERT_FILTER_SELECT(set) |
> > +				PS_Y_HORZ_FILTER_SELECT(set);
> > +
> > +	}
> > +	return scaler_filter_ctl;
> > +}
> > +
> 
> We don't want such copy pasta between planes and crtcs.

Yeah, got it. 
Will add a common enum drm_scaling_filter and use it.

Thanks,
Pankaj
 
> 
> >  static void
> >  skl_program_scaler(struct intel_plane *plane,
> >  		   const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7
> @@
> > skl_program_scaler(struct intel_plane *plane,
> >  	int scaler_id = plane_state->scaler_id;
> >  	const struct intel_scaler *scaler =
> >  		&crtc_state->scaler_state.scalers[scaler_id];
> > +	const struct drm_plane_state *state = &plane_state->uapi;
> >  	int crtc_x = plane_state->uapi.dst.x1;
> >  	int crtc_y = plane_state->uapi.dst.y1;
> >  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
> >  	u16 y_hphase, uv_rgb_hphase;
> >  	u16 y_vphase, uv_rgb_vphase;
> >  	int hscale, vscale;
> > +	int scaler_filter_ctl;
> >
> >  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> >  				      &plane_state->uapi.dst,
> > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
> >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> >  	}
> >
> > +	scaler_filter_ctl =
> > +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> > +					      state->scaling_filter);
> >  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> > -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-
> >mode);
> > +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> > +			  scaler->mode | scaler_filter_ctl);
> >  	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
> > +
> >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> >
> >  	return plane;
> > --
> > 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 v2 3/5] drm/i915: Introduce scaling filter related registers and bit fields.

[Ville Syrjälä]

On Tue, Mar 24, 2020 at 02:36:10PM +0000, Laxminarayan Bharadiya, Pankaj wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 23 March 2020 20:09
> > To: Laxminarayan Bharadiya, Pankaj
> > <pankaj.laxminarayan.bharadiya@intel.com>
> > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > David Airlie <airlied@linux.ie>
> > Subject: Re: [PATCH v2 3/5] drm/i915: Introduce scaling filter related registers
> > and bit fields.
> > 
> > On Thu, Mar 19, 2020 at 03:51:01PM +0530, Pankaj Bharadiya wrote:
> > > Introduce scaler registers and bit fields needed to configure the
> > > scaling filter in prgrammed mode and configure scaling filter
> > > coefficients.
> > >
> > > 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 | 48
> > > +++++++++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 9c53fe918be6..d40f12d2a6b5
> > > 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,25 @@ 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_COEF_SET1_INDEX_1A	   0x681A0
> > > +#define _PS_COEF_SET1_INDEX_2A	   0x682A0
> > > +#define _PS_COEF_SET1_INDEX_1B	   0x689A0
> > > +#define _PS_COEF_SET1_INDEX_2B	   0x68AA0
> > > +#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 _PS_COEF_SET1_DATA_1A	   0x681A4
> > > +#define _PS_COEF_SET1_DATA_2A	   0x682A4
> > > +#define _PS_COEF_SET1_DATA_1B	   0x689A4
> > > +#define _PS_COEF_SET1_DATA_2B	   0x68AA4
> > > +
> > >  #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),       \
> > > @@ -7310,6 +7334,30 @@ enum {
> > >  			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
> > >  			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))
> > >
> > > +#define _SKL_PS_COEF_INDEX_SET0(pipe, id)  _ID(pipe,    \
> > > +			_ID(id, _PS_COEF_SET0_INDEX_1A,
> > _PS_COEF_SET0_INDEX_2A), \
> > > +			_ID(id, _PS_COEF_SET0_INDEX_1B,
> > _PS_COEF_SET0_INDEX_2B))
> > > +
> > > +#define _SKL_PS_COEF_INDEX_SET1(pipe, id)  _ID(pipe,    \
> > > +			_ID(id, _PS_COEF_SET1_INDEX_1A,
> > _PS_COEF_SET1_INDEX_2A), \
> > > +			_ID(id, _PS_COEF_SET1_INDEX_1B,
> > _PS_COEF_SET1_INDEX_2B))
> > > +
> > > +#define _SKL_PS_COEF_DATA_SET0(pipe, id)  _ID(pipe,     \
> > > +			_ID(id, _PS_COEF_SET0_DATA_1A,
> > _PS_COEF_SET0_DATA_2A), \
> > > +			_ID(id, _PS_COEF_SET0_DATA_1B,
> > _PS_COEF_SET0_DATA_2B))
> > > +
> > > +#define _SKL_PS_COEF_DATA_SET1(pipe, id)  _ID(pipe,     \
> > > +			_ID(id, _PS_COEF_SET1_DATA_1A,
> > _PS_COEF_SET1_DATA_2A), \
> > > +			_ID(id, _PS_COEF_SET1_DATA_1B,
> > _PS_COEF_SET1_DATA_2B))
> > > +
> > > +#define SKL_PS_COEF_INDEX_SET(pipe, id, set) \
> > > +			_MMIO_PIPE(set, _SKL_PS_COEF_INDEX_SET0(pipe, id),
> > \
> > > +			    _SKL_PS_COEF_INDEX_SET1(pipe, id))
> > > +
> > > +#define SKL_PS_COEF_DATA_SET(pipe, id, set) \
> > > +			_MMIO_PIPE(set, _SKL_PS_COEF_DATA_SET0(pipe, id),
> > \
> > > +			    _SKL_PS_COEF_DATA_SET1(pipe, id))
> > 
> > I'd name those CNL_PS_COEF_{DATA,INDEX}(). Or maybe eeven GLK_ since it
> > already has this despite not being officially supported.
> 
> All other existing scaler macros start will  SKL_PS_*,  adding new CNL_PS_* may
> lead to confusion IMO since we will end up in using mixed SKL_PS_* and CNL_PS_* name
> to configure  scaler. 

They are called SKL_ because skl is where they got introduced.

> 
> > 
> > Also I'd probably just have used +(set)*8 instead of adding another trip through
> > _PICK_EVEN(). It's getting a bit hard to read this.
> 
> OK.
> How does this sound like?  -
> 
> +#define SKL_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 SKL_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)

Looks all right.

> 
> Thanks,
> Pankaj
> > 
> > > +
> > >  /* legacy palette */
> > >  #define _LGC_PALETTE_A           0x4a000
> > >  #define _LGC_PALETTE_B           0x4a800
> > > --
> > > 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

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

[Ville Syrjälä]

On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 23 March 2020 20:18
> > To: Laxminarayan Bharadiya, Pankaj
> > <pankaj.laxminarayan.bharadiya@intel.com>
> > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>;
> > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose
> > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
> > 
> > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> > > 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 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>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 32
> > > ++++++++++++++++++--  drivers/gpu/drm/i915/display/intel_sprite.c  |
> > > 31 ++++++++++++++++++-
> > >  2 files changed, 60 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 791dd908aa89..4b3387ee332e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6309,6 +6309,25 @@ void
> > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >
> > > +static u32
> > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > pipe,
> > > +			  int id, int set, enum drm_crtc_scaling_filter filter) {
> > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > +
> > > +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > +							 set);
> > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > +
> > > +	}
> > > +	return scaler_filter_ctl;
> > 
> > This function does too many things.
> 
> I was thinking to have a common function which configures the filter and also
> provides the register bits (ps_ctrl) to select a desired filter so that we need
> not have extra condition to figure out filter select register bits where this
> function is being called.
> How about renaming this function to some better name like  
> skl_scaler_set_and_get_filter_select() or something else? 
> Or shall I breakdown this function into multiple functions?

I'd just inline the PS_CTRL stuff into the current function.

> 
> Any suggestions?
> 
> > 
> > > +}
> > > +
> > >  static void skl_pfit_enable(const struct intel_crtc_state
> > > *crtc_state)  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct
> > intel_crtc_state *crtc_state)
> > >  	enum pipe pipe = crtc->pipe;
> > >  	const struct intel_crtc_scaler_state *scaler_state =
> > >  		&crtc_state->scaler_state;
> > > +	const struct drm_crtc_state *state = &crtc_state->uapi;
> > 
> > Pls don't add this kind of aliases. We're moving away from using the drm_ types
> > as much as possible.
> 
> OK.
> 
> > 
> > >
> > >  	if (crtc_state->pch_pfit.enabled) {
> > >  		u16 uv_rgb_hphase, uv_rgb_vphase;
> > >  		int pfit_w, pfit_h, hscale, vscale;
> > >  		unsigned long irqflags;
> > >  		int id;
> > > +		int scaler_filter_ctl;
> > 
> > It's a register value so u32. I'd also
> 
> Yes, I missed it. Thanks for pointing out.
> 
> > s/scaler_filter_ctl/filter_select/ or something like that.
> > 
> > Alternatively we could just call it ps_ctrl and have it contain the full register
> > value for that particular register.
> 
> ps_ctrl sounds better, will use this name.
> 
> > 
> > >
> > >  		if (drm_WARN_ON(&dev_priv->drm,
> > >  				crtc_state->scaler_state.scaler_id < 0)) @@ -
> > 6340,8 +6361,12 @@
> > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> > >
> > >  		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);
> > > +		scaler_filter_ctl =
> > > +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> > > +						state->scaling_filter);
> > > +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> > > +				  PS_SCALER_EN | scaler_filter_ctl |
> > > +				  scaler_state->scalers[id].mode);
> > >  		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), @@ -
> > 16777,6
> > > +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
> > > +
> > >  	intel_color_init(crtc);
> > >
> > >  	intel_crtc_crc_init(crtc);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index deda351719db..ac3fd9843ace 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
> > >  		return min(8192 * cpp, 32768);
> > >  }
> > >
> > > +static u32
> > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > pipe,
> > > +			      int id, int set,
> > > +			      enum drm_plane_scaling_filter filter) {
> > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > +
> > > +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > +							 set);
> > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > +
> > > +	}
> > > +	return scaler_filter_ctl;
> > > +}
> > > +
> > 
> > We don't want such copy pasta between planes and crtcs.
> 
> Yeah, got it. 
> Will add a common enum drm_scaling_filter and use it.
> 
> Thanks,
> Pankaj
>  
> > 
> > >  static void
> > >  skl_program_scaler(struct intel_plane *plane,
> > >  		   const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7
> > @@
> > > skl_program_scaler(struct intel_plane *plane,
> > >  	int scaler_id = plane_state->scaler_id;
> > >  	const struct intel_scaler *scaler =
> > >  		&crtc_state->scaler_state.scalers[scaler_id];
> > > +	const struct drm_plane_state *state = &plane_state->uapi;
> > >  	int crtc_x = plane_state->uapi.dst.x1;
> > >  	int crtc_y = plane_state->uapi.dst.y1;
> > >  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
> > >  	u16 y_hphase, uv_rgb_hphase;
> > >  	u16 y_vphase, uv_rgb_vphase;
> > >  	int hscale, vscale;
> > > +	int scaler_filter_ctl;
> > >
> > >  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> > >  				      &plane_state->uapi.dst,
> > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
> > >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > >  	}
> > >
> > > +	scaler_filter_ctl =
> > > +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> > > +					      state->scaling_filter);
> > >  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> > > -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-
> > >mode);
> > > +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> > > +			  scaler->mode | scaler_filter_ctl);
> > >  	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
> > > +
> > >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> > >
> > >  	return plane;
> > > --
> > > 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

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

[Bharadiya,Pankaj]

On Tue, Mar 24, 2020 at 06:46:10PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: 23 March 2020 20:18
> > > To: Laxminarayan Bharadiya, Pankaj
> > > <pankaj.laxminarayan.bharadiya@intel.com>
> > > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> > > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>;
> > > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose
> > > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma
> > > <uma.shankar@intel.com>
> > > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
> > > 
> > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> > > > 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 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>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 32
> > > > ++++++++++++++++++--  drivers/gpu/drm/i915/display/intel_sprite.c  |
> > > > 31 ++++++++++++++++++-
> > > >  2 files changed, 60 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 791dd908aa89..4b3387ee332e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6309,6 +6309,25 @@ void
> > > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> > > >  	}
> > > >  }
> > > >
> > > > +static u32
> > > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > > pipe,
> > > > +			  int id, int set, enum drm_crtc_scaling_filter filter) {
> > > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > > +
> > > > +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > > +							 set);
> > > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > > +
> > > > +	}
> > > > +	return scaler_filter_ctl;
> > > 
> > > This function does too many things.
> > 
> > I was thinking to have a common function which configures the filter and also
> > provides the register bits (ps_ctrl) to select a desired filter so that we need
> > not have extra condition to figure out filter select register bits where this
> > function is being called.
> > How about renaming this function to some better name like  
> > skl_scaler_set_and_get_filter_select() or something else? 
> > Or shall I breakdown this function into multiple functions?
> 
> I'd just inline the PS_CTRL stuff into the current function.

I am yet to verify this, but would like to get your early comments.
How about something like this? -

+inline u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set)
+{
+       u32 filter_select = PS_FILTER_MEDIUM;
+
+       if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
+               filter_select = PS_FILTER_PROGRAMMED |
+                       PS_UV_VERT_FILTER_SELECT(set) |
+                       PS_UV_HORZ_FILTER_SELECT(set) |
+                       PS_Y_VERT_FILTER_SELECT(set) |
+                       PS_Y_HORZ_FILTER_SELECT(set);
+       }
+
+       return filter_select;
+}
+
+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:
+       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);
@@ -6250,6 +6351,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))
@@ -6268,8 +6370,13 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)

                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->uapi.scaling_filter);
+
+               ps_ctrl = skl_scaler_get_filter_select(crtc_state->uapi.scaling_filter, 0);
+               ps_ctrl |=  PS_SCALER_EN | scaler_state->scalers[id].mode;
+
+               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),
@@ -16703,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;
        }

Thanks,
Pankaj

> 
> > 
> > Any suggestions?
> > 
> > > 
> > > > +}
> > > > +
> > > >  static void skl_pfit_enable(const struct intel_crtc_state
> > > > *crtc_state)  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct
> > > intel_crtc_state *crtc_state)
> > > >  	enum pipe pipe = crtc->pipe;
> > > >  	const struct intel_crtc_scaler_state *scaler_state =
> > > >  		&crtc_state->scaler_state;
> > > > +	const struct drm_crtc_state *state = &crtc_state->uapi;
> > > 
> > > Pls don't add this kind of aliases. We're moving away from using the drm_ types
> > > as much as possible.
> > 
> > OK.
> > 
> > > 
> > > >
> > > >  	if (crtc_state->pch_pfit.enabled) {
> > > >  		u16 uv_rgb_hphase, uv_rgb_vphase;
> > > >  		int pfit_w, pfit_h, hscale, vscale;
> > > >  		unsigned long irqflags;
> > > >  		int id;
> > > > +		int scaler_filter_ctl;
> > > 
> > > It's a register value so u32. I'd also
> > 
> > Yes, I missed it. Thanks for pointing out.
> > 
> > > s/scaler_filter_ctl/filter_select/ or something like that.
> > > 
> > > Alternatively we could just call it ps_ctrl and have it contain the full register
> > > value for that particular register.
> > 
> > ps_ctrl sounds better, will use this name.
> > 
> > > 
> > > >
> > > >  		if (drm_WARN_ON(&dev_priv->drm,
> > > >  				crtc_state->scaler_state.scaler_id < 0)) @@ -
> > > 6340,8 +6361,12 @@
> > > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> > > >
> > > >  		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);
> > > > +		scaler_filter_ctl =
> > > > +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> > > > +						state->scaling_filter);
> > > > +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> > > > +				  PS_SCALER_EN | scaler_filter_ctl |
> > > > +				  scaler_state->scalers[id].mode);
> > > >  		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), @@ -
> > > 16777,6
> > > > +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
> > > > +
> > > >  	intel_color_init(crtc);
> > > >
> > > >  	intel_crtc_crc_init(crtc);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index deda351719db..ac3fd9843ace 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
> > > >  		return min(8192 * cpp, 32768);
> > > >  }
> > > >
> > > > +static u32
> > > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > > pipe,
> > > > +			      int id, int set,
> > > > +			      enum drm_plane_scaling_filter filter) {
> > > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > > +
> > > > +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > > +							 set);
> > > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > > +
> > > > +	}
> > > > +	return scaler_filter_ctl;
> > > > +}
> > > > +
> > > 
> > > We don't want such copy pasta between planes and crtcs.
> > 
> > Yeah, got it. 
> > Will add a common enum drm_scaling_filter and use it.
> > 
> > Thanks,
> > Pankaj
> >  
> > > 
> > > >  static void
> > > >  skl_program_scaler(struct intel_plane *plane,
> > > >  		   const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7
> > > @@
> > > > skl_program_scaler(struct intel_plane *plane,
> > > >  	int scaler_id = plane_state->scaler_id;
> > > >  	const struct intel_scaler *scaler =
> > > >  		&crtc_state->scaler_state.scalers[scaler_id];
> > > > +	const struct drm_plane_state *state = &plane_state->uapi;
> > > >  	int crtc_x = plane_state->uapi.dst.x1;
> > > >  	int crtc_y = plane_state->uapi.dst.y1;
> > > >  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> > > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
> > > >  	u16 y_hphase, uv_rgb_hphase;
> > > >  	u16 y_vphase, uv_rgb_vphase;
> > > >  	int hscale, vscale;
> > > > +	int scaler_filter_ctl;
> > > >
> > > >  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> > > >  				      &plane_state->uapi.dst,
> > > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
> > > >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > > >  	}
> > > >
> > > > +	scaler_filter_ctl =
> > > > +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> > > > +					      state->scaling_filter);
> > > >  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> > > > -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-
> > > >mode);
> > > > +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> > > > +			  scaler->mode | scaler_filter_ctl);
> > > >  	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
> > > > +
> > > >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> > > >
> > > >  	return plane;
> > > > --
> > > > 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

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

[Ville Syrjälä]

On Thu, Mar 26, 2020 at 08:45:59PM +0530, Bharadiya,Pankaj wrote:
> On Tue, Mar 24, 2020 at 06:46:10PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Sent: 23 March 2020 20:18
> > > > To: Laxminarayan Bharadiya, Pankaj
> > > > <pankaj.laxminarayan.bharadiya@intel.com>
> > > > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> > > > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > > > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > > > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>;
> > > > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose
> > > > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma
> > > > <uma.shankar@intel.com>
> > > > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
> > > > 
> > > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> > > > > 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 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>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 32
> > > > > ++++++++++++++++++--  drivers/gpu/drm/i915/display/intel_sprite.c  |
> > > > > 31 ++++++++++++++++++-
> > > > >  2 files changed, 60 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 791dd908aa89..4b3387ee332e 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -6309,6 +6309,25 @@ void
> > > > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> > > > >  	}
> > > > >  }
> > > > >
> > > > > +static u32
> > > > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > > > pipe,
> > > > > +			  int id, int set, enum drm_crtc_scaling_filter filter) {
> > > > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > > > +
> > > > > +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > > > +							 set);
> > > > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > > > +
> > > > > +	}
> > > > > +	return scaler_filter_ctl;
> > > > 
> > > > This function does too many things.
> > > 
> > > I was thinking to have a common function which configures the filter and also
> > > provides the register bits (ps_ctrl) to select a desired filter so that we need
> > > not have extra condition to figure out filter select register bits where this
> > > function is being called.
> > > How about renaming this function to some better name like  
> > > skl_scaler_set_and_get_filter_select() or something else? 
> > > Or shall I breakdown this function into multiple functions?
> > 
> > I'd just inline the PS_CTRL stuff into the current function.
> 
> I am yet to verify this, but would like to get your early comments.
> How about something like this? -
> 
> +inline u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set)
> +{
> +       u32 filter_select = PS_FILTER_MEDIUM;

Pointless variable. 

if (whatever)
	return A;
else
	return B;

> +
> +       if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> +               filter_select = PS_FILTER_PROGRAMMED |
> +                       PS_UV_VERT_FILTER_SELECT(set) |
> +                       PS_UV_HORZ_FILTER_SELECT(set) |
> +                       PS_Y_VERT_FILTER_SELECT(set) |
> +                       PS_Y_HORZ_FILTER_SELECT(set);
> +       }
> +
> +       return filter_select;
> +}
> +
> +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:
> +       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);
> @@ -6250,6 +6351,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))
> @@ -6268,8 +6370,13 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> 
>                 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->uapi.scaling_filter);

btw we need to duplicate the scaling filter in the hw state.

> +
> +               ps_ctrl = skl_scaler_get_filter_select(crtc_state->uapi.scaling_filter, 0);
> +               ps_ctrl |=  PS_SCALER_EN | scaler_state->scalers[id].mode;

This can all be done outside the spinlock.

Otherwise lgtm


> +
> +               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),
> @@ -16703,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;
>         }
> 
> Thanks,
> Pankaj
> 
> > 
> > > 
> > > Any suggestions?
> > > 
> > > > 
> > > > > +}
> > > > > +
> > > > >  static void skl_pfit_enable(const struct intel_crtc_state
> > > > > *crtc_state)  {
> > > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct
> > > > intel_crtc_state *crtc_state)
> > > > >  	enum pipe pipe = crtc->pipe;
> > > > >  	const struct intel_crtc_scaler_state *scaler_state =
> > > > >  		&crtc_state->scaler_state;
> > > > > +	const struct drm_crtc_state *state = &crtc_state->uapi;
> > > > 
> > > > Pls don't add this kind of aliases. We're moving away from using the drm_ types
> > > > as much as possible.
> > > 
> > > OK.
> > > 
> > > > 
> > > > >
> > > > >  	if (crtc_state->pch_pfit.enabled) {
> > > > >  		u16 uv_rgb_hphase, uv_rgb_vphase;
> > > > >  		int pfit_w, pfit_h, hscale, vscale;
> > > > >  		unsigned long irqflags;
> > > > >  		int id;
> > > > > +		int scaler_filter_ctl;
> > > > 
> > > > It's a register value so u32. I'd also
> > > 
> > > Yes, I missed it. Thanks for pointing out.
> > > 
> > > > s/scaler_filter_ctl/filter_select/ or something like that.
> > > > 
> > > > Alternatively we could just call it ps_ctrl and have it contain the full register
> > > > value for that particular register.
> > > 
> > > ps_ctrl sounds better, will use this name.
> > > 
> > > > 
> > > > >
> > > > >  		if (drm_WARN_ON(&dev_priv->drm,
> > > > >  				crtc_state->scaler_state.scaler_id < 0)) @@ -
> > > > 6340,8 +6361,12 @@
> > > > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> > > > >
> > > > >  		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);
> > > > > +		scaler_filter_ctl =
> > > > > +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> > > > > +						state->scaling_filter);
> > > > > +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> > > > > +				  PS_SCALER_EN | scaler_filter_ctl |
> > > > > +				  scaler_state->scalers[id].mode);
> > > > >  		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), @@ -
> > > > 16777,6
> > > > > +16802,9 @@ 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_enable_scaling_filter(&crtc->base);
> > > > > +
> > > > >  	intel_color_init(crtc);
> > > > >
> > > > >  	intel_crtc_crc_init(crtc);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > index deda351719db..ac3fd9843ace 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
> > > > >  		return min(8192 * cpp, 32768);
> > > > >  }
> > > > >
> > > > > +static u32
> > > > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> > > > pipe,
> > > > > +			      int id, int set,
> > > > > +			      enum drm_plane_scaling_filter filter) {
> > > > > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > > > > +
> > > > > +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > > > > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > > > > +							 set);
> > > > > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > > > > +				PS_UV_VERT_FILTER_SELECT(set) |
> > > > > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > > > > +				PS_Y_VERT_FILTER_SELECT(set) |
> > > > > +				PS_Y_HORZ_FILTER_SELECT(set);
> > > > > +
> > > > > +	}
> > > > > +	return scaler_filter_ctl;
> > > > > +}
> > > > > +
> > > > 
> > > > We don't want such copy pasta between planes and crtcs.
> > > 
> > > Yeah, got it. 
> > > Will add a common enum drm_scaling_filter and use it.
> > > 
> > > Thanks,
> > > Pankaj
> > >  
> > > > 
> > > > >  static void
> > > > >  skl_program_scaler(struct intel_plane *plane,
> > > > >  		   const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7
> > > > @@
> > > > > skl_program_scaler(struct intel_plane *plane,
> > > > >  	int scaler_id = plane_state->scaler_id;
> > > > >  	const struct intel_scaler *scaler =
> > > > >  		&crtc_state->scaler_state.scalers[scaler_id];
> > > > > +	const struct drm_plane_state *state = &plane_state->uapi;
> > > > >  	int crtc_x = plane_state->uapi.dst.x1;
> > > > >  	int crtc_y = plane_state->uapi.dst.y1;
> > > > >  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> > > > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
> > > > >  	u16 y_hphase, uv_rgb_hphase;
> > > > >  	u16 y_vphase, uv_rgb_vphase;
> > > > >  	int hscale, vscale;
> > > > > +	int scaler_filter_ctl;
> > > > >
> > > > >  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> > > > >  				      &plane_state->uapi.dst,
> > > > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
> > > > >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > > > >  	}
> > > > >
> > > > > +	scaler_filter_ctl =
> > > > > +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> > > > > +					      state->scaling_filter);
> > > > >  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> > > > > -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-
> > > > >mode);
> > > > > +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> > > > > +			  scaler->mode | scaler_filter_ctl);
> > > > >  	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 +3147,9 @@ 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_enable_scaling_filter(&plane->base);
> > > > > +
> > > > >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> > > > >
> > > > >  	return plane;
> > > > > --
> > > > > 2.23.0
> > > > 
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > 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