[PATCH v3 0/2] Add writeback scaling

[PATCH v3 0/2] Add writeback scaling

[Ben Davis]

Add support for scaling on writeback. To do this add writeback_dest_w
and writeback_dest_h as writeback connector properties to specify the
desired output dimensions.
Then implement downscaling on writeback for Malidp-550 and Malidp-650
(upscaling on writeback is not supported on these devices).

v2: Use 0 as default for writeback_w,h and so update range to use 1 as
    minimum.

v3: Rename properties to specify they are destination width/height.
    Make sure the values from the properties are passed to
    enable_memwrite rather than the framebuffer dimensions

Ben Davis (2):
  drm: Add writeback_dest_w,h properties
  drm/malidp: Enable writeback scaling

 drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++---------
 drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++
 drivers/gpu/drm/arm/malidp_drv.c  | 10 +++--
 drivers/gpu/drm/arm/malidp_hw.c   | 45 +++++++++++++------
 drivers/gpu/drm/arm/malidp_hw.h   | 19 ++++++--
 drivers/gpu/drm/arm/malidp_mw.c   | 94 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
 drivers/gpu/drm/drm_writeback.c   | 30 +++++++++++++
 include/drm/drm_connector.h       |  4 ++
 include/drm/drm_mode_config.h     | 10 +++++
 11 files changed, 220 insertions(+), 62 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h

-- 
2.7.4

[PATCH v3 1/2] drm: Add writeback_dest_w,h properties

[Ben Davis]

Add new properties to specify width and height for writeback.

Signed-off-by: Ben Davis <ben.davis@arm.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++++++
 drivers/gpu/drm/drm_writeback.c   | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  4 ++++
 include/drm/drm_mode_config.h     | 10 ++++++++++
 4 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d520a04..064d986 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -765,6 +765,10 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 			return -EINVAL;
 		}
 		state->content_protection = val;
+	} else if (property == config->prop_writeback_dest_w) {
+		state->writeback_dest_w = val;
+	} else if (property == config->prop_writeback_dest_h) {
+		state->writeback_dest_h = val;
 	} else if (property == config->writeback_fb_id_property) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
 		int ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
@@ -837,6 +841,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->scaling_mode;
 	} else if (property == connector->content_protection_property) {
 		*val = state->content_protection;
+	} else if (property == config->prop_writeback_dest_w) {
+		*val = state->writeback_dest_w;
+	} else if (property == config->prop_writeback_dest_h) {
+		*val = state->writeback_dest_h;
 	} else if (property == config->writeback_fb_id_property) {
 		/* Writeback framebuffer is one-shot, write and forget */
 		*val = 0;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index c20e6fe..2d7ee22 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -74,6 +74,14 @@
  *	applications making use of writeback connectors *always* retrieve an
  *	out-fence for the commit and use it appropriately.
  *	From userspace, this property will always read as zero.
+ *
+ *  "WRITEBACK_DEST_W":
+ *	The width of the composition to write back. 0 acts as default. If
+ *	non-zero the composition will be scaled to match the given width.
+ *
+ *  "WRITEBACK_DEST_H":
+ *	The height of the composition to write back. 0 acts as default. If
+ *	non-zero the composition will be scaled to match the given height.
  */
 
 #define fence_to_wb_connector(x) container_of(x->lock, \
@@ -141,6 +149,22 @@ static int create_writeback_properties(struct drm_device *dev)
 		dev->mode_config.writeback_out_fence_ptr_property = prop;
 	}
 
+	if (!dev->mode_config.prop_writeback_dest_w) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_DEST_W", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_dest_w = prop;
+	}
+
+	if (!dev->mode_config.prop_writeback_dest_h) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_DEST_H", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_dest_h = prop;
+	}
+
 	return 0;
 }
 
@@ -225,6 +249,12 @@ int drm_writeback_connector_init(struct drm_device *dev,
 	drm_object_attach_property(&connector->base,
 				   config->writeback_pixel_formats_property,
 				   blob->base.id);
+
+	drm_object_attach_property(&connector->base,
+				   config->prop_writeback_dest_w, 0);
+	drm_object_attach_property(&connector->base,
+				   config->prop_writeback_dest_h, 0);
+
 	wb_connector->pixel_formats_blob_ptr = blob;
 
 	return 0;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8fe22ab..c956960 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -515,6 +515,10 @@ struct drm_connector_state {
 	 */
 	struct drm_writeback_job *writeback_job;
 
+	/** @writeback_dest_w: width of plane to write to wb buffer */
+	/** @writeback_dest_h: height of plane to write to wb buffer */
+	uint32_t writeback_dest_w, writeback_dest_h;
+
 	/**
 	 * @max_requested_bpc: Connector property to limit the maximum bit
 	 * depth of the pixels.
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7f60e8e..5e30a58 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -622,6 +622,16 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_crtc_h;
 	/**
+	 * @prop_writeback_dest_w: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_dest_w;
+	/**
+	 * @prop_writeback_dest_h: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_dest_h;
+	/**
 	 * @prop_fb_id: Default atomic plane property to specify the
 	 * &drm_framebuffer.
 	 */
-- 
2.7.4

[PATCH v3 2/2] drm/malidp: Enable writeback scaling

[Ben Davis]

The phase setting part of malidp_crtc_atomic_check_scaling is refactored
to allow use in writeback scaling.

Also the enable_memwrite function prototype is simplified by directly
passing mw_state.

Signed-off-by: Ben Davis <ben.davis@arm.com>
---
 drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++---------
 drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++
 drivers/gpu/drm/arm/malidp_drv.c  | 10 +++--
 drivers/gpu/drm/arm/malidp_hw.c   | 45 +++++++++++++------
 drivers/gpu/drm/arm/malidp_hw.h   | 19 ++++++--
 drivers/gpu/drm/arm/malidp_mw.c   | 94 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 7 files changed, 168 insertions(+), 62 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h

diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 3f65996..aa66457 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -265,6 +265,31 @@ static int malidp_crtc_atomic_check_ctm(struct drm_crtc *crtc,
 	return 0;
 }
 
+void malidp_set_se_config_phase(struct malidp_se_config *s)
+{
+#define SE_N_PHASE 4
+#define SE_SHIFT_N_PHASE 12
+	u32 phase;
+
+	/* Calculate initial_phase and delta_phase for horizontal. */
+	phase = s->input_w;
+	s->h_init_phase = ((phase << SE_N_PHASE) / s->output_w + 1) / 2;
+
+	phase = s->input_w;
+	phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE);
+	s->h_delta_phase = phase / s->output_w;
+
+	/* Same for vertical. */
+	phase = s->input_h;
+	s->v_init_phase = ((phase << SE_N_PHASE) / s->output_h + 1) / 2;
+
+	phase = s->input_h;
+	phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE);
+	s->v_delta_phase = phase / s->output_h;
+#undef SE_N_PHASE
+#undef SE_SHIFT_N_PHASE
+}
+
 static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
 					    struct drm_crtc_state *state)
 {
@@ -291,7 +316,6 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
 
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
 		struct malidp_plane *mp = to_malidp_plane(plane);
-		u32 phase;
 
 		if (!(mp->layer->id & scaling))
 			continue;
@@ -319,27 +343,8 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
 		s->output_w = pstate->crtc_w;
 		s->output_h = pstate->crtc_h;
 
-#define SE_N_PHASE 4
-#define SE_SHIFT_N_PHASE 12
-		/* Calculate initial_phase and delta_phase for horizontal. */
-		phase = s->input_w;
-		s->h_init_phase =
-				((phase << SE_N_PHASE) / s->output_w + 1) / 2;
-
-		phase = s->input_w;
-		phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE);
-		s->h_delta_phase = phase / s->output_w;
-
-		/* Same for vertical. */
-		phase = s->input_h;
-		s->v_init_phase =
-				((phase << SE_N_PHASE) / s->output_h + 1) / 2;
-
-		phase = s->input_h;
-		phase <<= (SE_SHIFT_N_PHASE + SE_N_PHASE);
-		s->v_delta_phase = phase / s->output_h;
-#undef SE_N_PHASE
-#undef SE_SHIFT_N_PHASE
+
+		malidp_set_se_config_phase(s);
 		s->plane_src_id = mp->layer->id;
 	}
 
diff --git a/drivers/gpu/drm/arm/malidp_crtc.h b/drivers/gpu/drm/arm/malidp_crtc.h
new file mode 100644
index 0000000..0f056f8
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_crtc.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * (C) COPYRIGHT 2019 ARM Limited. All rights reserved.
+ *
+ */
+
+#include "malidp_hw.h"
+
+#ifndef __MALIDP_CRTC_H__
+#define __MALIDP_CRTC_H__
+void malidp_set_se_config_phase(struct malidp_se_config *s);
+#endif
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d37ff9d..e2465e9 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -138,7 +138,7 @@ static void malidp_atomic_commit_se_config(struct drm_crtc *crtc,
 	u32 val;
 
 	/* Set SE_CONTROL */
-	if (!s->scale_enable) {
+	if (!s->scale_enable && !s->wb_scale_enable) {
 		val = malidp_hw_read(hwdev, se_control);
 		val &= ~MALIDP_SE_SCALING_EN;
 		malidp_hw_write(hwdev, val, se_control);
@@ -147,12 +147,16 @@ static void malidp_atomic_commit_se_config(struct drm_crtc *crtc,
 
 	hwdev->hw->se_set_scaling_coeffs(hwdev, s, old_s);
 	val = malidp_hw_read(hwdev, se_control);
-	val |= MALIDP_SE_SCALING_EN | MALIDP_SE_ALPHA_EN;
+	val |= MALIDP_SE_SCALING_EN;
 
 	val &= ~MALIDP_SE_ENH(MALIDP_SE_ENH_MASK);
 	val |= s->enhancer_enable ? MALIDP_SE_ENH(3) : 0;
 
-	val |= MALIDP_SE_RGBO_IF_EN;
+	if (s->scale_enable)
+		val |= (MALIDP_SE_RGBO_IF_EN | MALIDP_SE_ALPHA_EN);
+	else
+		val &= ~((MALIDP_SE_RGBO_IF_EN | MALIDP_SE_ALPHA_EN));
+
 	malidp_hw_write(hwdev, val, se_control);
 
 	/* Set IN_SIZE & OUT_SIZE. */
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 1a36718..6f59c0e 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -497,13 +497,18 @@ static long malidp500_se_calc_mclk(struct malidp_hw_device *hwdev,
 	return ret;
 }
 
-static int malidp500_enable_memwrite(struct malidp_hw_device *hwdev,
-				     dma_addr_t *addrs, s32 *pitches,
-				     int num_planes, u16 w, u16 h, u32 fmt_id,
-				     const s16 *rgb2yuv_coeffs)
+static int malidp500_enable_memwrite(struct malidp_hw_device *hwdev, u16 w, u16 h,
+				     struct malidp_mw_connector_state *mw_state)
 {
+
+	int num_planes = mw_state->n_planes;
+	dma_addr_t *addrs = mw_state->addrs;
+	s32 *pitches = mw_state->pitches;
+	u32 fmt_id = mw_state->format;
 	u32 base = MALIDP500_SE_MEMWRITE_BASE;
 	u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+	const s16 *rgb2yuv_coeffs = !mw_state->rgb2yuv_initialized ?
+					mw_state->rgb2yuv_coeffs : NULL;
 
 	/* enable the scaling engine block */
 	malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC);
@@ -847,13 +852,18 @@ static long malidp550_se_calc_mclk(struct malidp_hw_device *hwdev,
 	return ret;
 }
 
-static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
-				     dma_addr_t *addrs, s32 *pitches,
-				     int num_planes, u16 w, u16 h, u32 fmt_id,
-				     const s16 *rgb2yuv_coeffs)
+static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev, u16 w, u16 h,
+				     struct malidp_mw_connector_state *mw_state)
 {
+	int num_planes = mw_state->n_planes;
+	bool scaling = mw_state->wb_scale_enable;
+	dma_addr_t *addrs = mw_state->addrs;
+	s32 *pitches = mw_state->pitches;
+	u32 fmt_id = mw_state->format;
 	u32 base = MALIDP550_SE_MEMWRITE_BASE;
 	u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+	const s16 *rgb2yuv_coeffs = !mw_state->rgb2yuv_initialized ?
+					mw_state->rgb2yuv_coeffs : NULL;
 
 	/* enable the scaling engine block */
 	malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC);
@@ -876,10 +886,18 @@ static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
 		WARN(1, "Invalid number of planes");
 	}
 
-	malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h),
-			MALIDP550_SE_MEMWRITE_OUT_SIZE);
-	malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
-			  MALIDP550_SE_CONTROL);
+	malidp_hw_clearbits(hwdev, MALIDP_SE_MEMWRITE_EN | MALIDP_SE_MEMWRITE_SCL_EN,
+			    MALIDP550_SE_CONTROL);
+
+	if (scaling) {
+		malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_SCL_EN,
+				  MALIDP550_SE_CONTROL);
+	} else {
+		malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h),
+				MALIDP550_SE_MEMWRITE_OUT_SIZE);
+		malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+				  MALIDP550_SE_CONTROL);
+	}
 
 	if (rgb2yuv_coeffs) {
 		int i;
@@ -897,7 +915,8 @@ static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev)
 {
 	u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
 
-	malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+	malidp_hw_clearbits(hwdev,
+			    MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN | MALIDP_SE_MEMWRITE_SCL_EN,
 			    MALIDP550_SE_CONTROL);
 	malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC);
 }
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 8352a2e..61ddcb5 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -85,6 +85,7 @@ enum malidp_scaling_coeff_set {
 struct malidp_se_config {
 	u8 scale_enable : 1;
 	u8 enhancer_enable : 1;
+	u8 wb_scale_enable : 1;
 	u8 hcoeff : 3;
 	u8 vcoeff : 3;
 	u8 plane_src_id;
@@ -94,6 +95,19 @@ struct malidp_se_config {
 	u32 v_init_phase, v_delta_phase;
 };
 
+#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state)
+
+struct malidp_mw_connector_state {
+	struct drm_connector_state base;
+	dma_addr_t addrs[2];
+	s32 pitches[2];
+	u8 format;
+	u8 n_planes;
+	bool rgb2yuv_initialized;
+	const s16 *rgb2yuv_coeffs;
+	bool wb_scale_enable;
+};
+
 /* regmap features */
 #define MALIDP_REGMAP_HAS_CLEARIRQ				BIT(0)
 #define MALIDP_DEVICE_AFBC_SUPPORT_SPLIT			BIT(1)
@@ -206,9 +220,8 @@ struct malidp_hw {
 	 * @param h - height of the output frame
 	 * @param fmt_id - internal format ID of output buffer
 	 */
-	int (*enable_memwrite)(struct malidp_hw_device *hwdev, dma_addr_t *addrs,
-			       s32 *pitches, int num_planes, u16 w, u16 h, u32 fmt_id,
-			       const s16 *rgb2yuv_coeffs);
+	int (*enable_memwrite)(struct malidp_hw_device *hwdev, u16 w, u16 h,
+			       struct malidp_mw_connector_state *mw_state);
 
 	/*
 	 * Disable the writing to memory of the next frame's content.
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index 2865f7a..1e1f47c 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -13,23 +13,13 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drmP.h>
 #include <drm/drm_writeback.h>
+#include <video/videomode.h>
 
+#include "malidp_crtc.h"
 #include "malidp_drv.h"
 #include "malidp_hw.h"
 #include "malidp_mw.h"
 
-#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state)
-
-struct malidp_mw_connector_state {
-	struct drm_connector_state base;
-	dma_addr_t addrs[2];
-	s32 pitches[2];
-	u8 format;
-	u8 n_planes;
-	bool rgb2yuv_initialized;
-	const s16 *rgb2yuv_coeffs;
-};
-
 static int malidp_mw_connector_get_modes(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
@@ -126,16 +116,82 @@ malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
 			       struct drm_connector_state *conn_state)
 {
 	struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state);
+	struct malidp_crtc_state *cs = to_malidp_crtc_state(crtc_state);
+	struct malidp_se_config *s = &cs->scaler_config;
 	struct malidp_drm *malidp = encoder->dev->dev_private;
 	struct drm_framebuffer *fb;
-	int i, n_planes;
+	int i, n_planes, ret;
+	struct malidp_hw_device *hwdev = malidp->dev;
+	struct videomode vm;
+	u32 h_upscale_factor = 0; /* U16.16 */
+	u32 v_upscale_factor = 0; /* U16.16 */
+
+	s->wb_scale_enable = 0;
 
 	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
 		return 0;
 
+
+	/* 0s are the default for writeback_dest_w,h - so don't scale.
+	 * The case where one of w,h is set but the other is 0 is meaningless
+	 * so ignore it
+	 */
+	if (conn_state->writeback_dest_w && conn_state->writeback_dest_h) {
+		if ((conn_state->writeback_dest_w != crtc_state->mode.hdisplay) ||
+		    (conn_state->writeback_dest_h != crtc_state->mode.vdisplay)) {
+			s->wb_scale_enable = 1;
+			mw_state->wb_scale_enable = true;
+		}
+	}
+
 	fb = conn_state->writeback_job->fb;
-	if ((fb->width != crtc_state->mode.hdisplay) ||
-	    (fb->height != crtc_state->mode.vdisplay)) {
+	if (s->wb_scale_enable) {
+		if (s->scale_enable) {
+			DRM_DEBUG_KMS("Attempting to scale on writeback while scaling a layer\n");
+			return -EINVAL;
+		}
+
+		s->input_w = crtc_state->mode.hdisplay;
+		s->input_h = crtc_state->mode.vdisplay;
+		s->output_w = conn_state->writeback_dest_w;
+		s->output_h = conn_state->writeback_dest_h;
+
+		if (fb->width < s->output_w || fb->height < s->output_h) {
+			DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+					fb->width, fb->height);
+			return -EINVAL;
+		}
+
+		if ((s->output_w > s->input_w) || (s->output_h > s->input_h)) {
+			DRM_DEBUG_KMS("Upscaling on writeback is not supported\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * Convert crtc_[w|h] to U32.32, then divide by U16.16 src_[w|h]
+		 * to get the U16.16 result.
+		 */
+		h_upscale_factor = div_u64((u64)s->output_w << 32,
+					   (u32)s->input_w << 16);
+		v_upscale_factor = div_u64((u64)s->output_h << 32,
+					   (u32)s->input_h << 16);
+
+		/* enhancer is for upscaling */
+		s->enhancer_enable = 0;
+
+		malidp_set_se_config_phase(s);
+
+		s->hcoeff = malidp_se_select_coeffs(h_upscale_factor);
+		s->vcoeff = malidp_se_select_coeffs(v_upscale_factor);
+
+		drm_display_mode_to_videomode(&crtc_state->adjusted_mode, &vm);
+		ret = hwdev->hw->se_calc_mclk(hwdev, s, &vm);
+		if (ret < 0)
+			return -EINVAL;
+	}
+
+	if (!s->wb_scale_enable && ((fb->width != crtc_state->mode.hdisplay) ||
+		       (fb->height != crtc_state->mode.vdisplay))) {
 		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
 				fb->width, fb->height);
 		return -EINVAL;
@@ -252,18 +308,14 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
 
 		DRM_DEV_DEBUG_DRIVER(drm->dev,
 				     "Enable memwrite %ux%u:%d %pad fmt: %u\n",
-				     fb->width, fb->height,
+				     conn_state->writeback_dest_w, conn_state->writeback_dest_h,
 				     mw_state->pitches[0],
 				     &mw_state->addrs[0],
 				     mw_state->format);
 
 		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
 		conn_state->writeback_job = NULL;
-		hwdev->hw->enable_memwrite(hwdev, mw_state->addrs,
-					   mw_state->pitches, mw_state->n_planes,
-					   fb->width, fb->height, mw_state->format,
-					   !mw_state->rgb2yuv_initialized ?
-					   mw_state->rgb2yuv_coeffs : NULL);
+		hwdev->hw->enable_memwrite(hwdev, fb->width, fb->height, mw_state);
 		mw_state->rgb2yuv_initialized = !!mw_state->rgb2yuv_coeffs;
 	} else {
 		DRM_DEV_DEBUG_DRIVER(drm->dev, "Disable memwrite\n");
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index a3363f3..254aa98 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -73,6 +73,7 @@
 #define   MALIDP_DISP_FUNC_CADJ		(1 << 4)
 #define   MALIDP_DISP_FUNC_ILACED	(1 << 8)
 #define   MALIDP_SCALE_ENGINE_EN	(1 << 16)
+#define   MALIDP_SE_MEMWRITE_SCL_EN	(1 << 5)
 #define   MALIDP_SE_MEMWRITE_EN		(2 << 5)
 
 /* register offsets for IRQ management */
-- 
2.7.4

Re: [PATCH v3 0/2] Add writeback scaling

[Daniel Vetter]

On Tue, Apr 23, 2019 at 12:48 PM Ben Davis <Ben.Davis@arm.com> wrote:
>
> Add support for scaling on writeback. To do this add writeback_dest_w
> and writeback_dest_h as writeback connector properties to specify the
> desired output dimensions.
> Then implement downscaling on writeback for Malidp-550 and Malidp-650
> (upscaling on writeback is not supported on these devices).
>
> v2: Use 0 as default for writeback_w,h and so update range to use 1 as
>     minimum.

Hm I missed that, I don't think that's good, since it prevents
userspace from blindly writing back the properties it's read. We've
tried hard to avoid that, since we're suggesting compositor can take a
snapshot of all kms properties (including the ones they don't
understand) and restore that on vt switching. Hence stuff like fence
fds returning -1, and accepting -1 as NULL to make this work.

tldr; I think range needs to include 0, and we need make that a
special thing, maybe enforced with a
drm_connector_state_compute_writeback_dst_h/w, which takes
crtc_state->mode.v/hdisplay into account if the writeback_dst_h/w is
0.
-Daniel

>
> v3: Rename properties to specify they are destination width/height.
>     Make sure the values from the properties are passed to
>     enable_memwrite rather than the framebuffer dimensions
>
> Ben Davis (2):
>   drm: Add writeback_dest_w,h properties
>   drm/malidp: Enable writeback scaling
>
>  drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++---------
>  drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++
>  drivers/gpu/drm/arm/malidp_drv.c  | 10 +++--
>  drivers/gpu/drm/arm/malidp_hw.c   | 45 +++++++++++++------
>  drivers/gpu/drm/arm/malidp_hw.h   | 19 ++++++--
>  drivers/gpu/drm/arm/malidp_mw.c   | 94 ++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/arm/malidp_regs.h |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
>  drivers/gpu/drm/drm_writeback.c   | 30 +++++++++++++
>  include/drm/drm_connector.h       |  4 ++
>  include/drm/drm_mode_config.h     | 10 +++++
>  11 files changed, 220 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h
>
> --
> 2.7.4
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 0/2] Add writeback scaling

[Liviu Dudau]

On Tue, Apr 23, 2019 at 02:24:11PM +0200, Daniel Vetter wrote:
> On Tue, Apr 23, 2019 at 12:48 PM Ben Davis <Ben.Davis@arm.com> wrote:
> >
> > Add support for scaling on writeback. To do this add writeback_dest_w
> > and writeback_dest_h as writeback connector properties to specify the
> > desired output dimensions.
> > Then implement downscaling on writeback for Malidp-550 and Malidp-650
> > (upscaling on writeback is not supported on these devices).
> >
> > v2: Use 0 as default for writeback_w,h and so update range to use 1 as
> >     minimum.
> 
> Hm I missed that, I don't think that's good, since it prevents
> userspace from blindly writing back the properties it's read. We've
> tried hard to avoid that, since we're suggesting compositor can take a
> snapshot of all kms properties (including the ones they don't
> understand) and restore that on vt switching. Hence stuff like fence
> fds returning -1, and accepting -1 as NULL to make this work.

Is that they way it should work, though? I remember a few good years
back, before KMS, there were lots of issues with X server hanging and
not restoring the vt mode correctly. Should it not be that on getting
back control of a DRM master, an application sets it last known good
mode that it had, before it lost (or gave away) control?

> 
> tldr; I think range needs to include 0, and we need make that a
> special thing, maybe enforced with a
> drm_connector_state_compute_writeback_dst_h/w, which takes
> crtc_state->mode.v/hdisplay into account if the writeback_dst_h/w is
> 0.

Repeating just to make sure we're on the same page: we allow 0 as valid
range value, but writeback_dst_h/w ultimately gets updated before the
commit gets passed on to the driver to use crtc_state->mode.v/hdisplay
so as not to trigger any scaling. Correct?

Best regards,
Liviu


> -Daniel
> 
> >
> > v3: Rename properties to specify they are destination width/height.
> >     Make sure the values from the properties are passed to
> >     enable_memwrite rather than the framebuffer dimensions
> >
> > Ben Davis (2):
> >   drm: Add writeback_dest_w,h properties
> >   drm/malidp: Enable writeback scaling
> >
> >  drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++---------
> >  drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++
> >  drivers/gpu/drm/arm/malidp_drv.c  | 10 +++--
> >  drivers/gpu/drm/arm/malidp_hw.c   | 45 +++++++++++++------
> >  drivers/gpu/drm/arm/malidp_hw.h   | 19 ++++++--
> >  drivers/gpu/drm/arm/malidp_mw.c   | 94 ++++++++++++++++++++++++++++++---------
> >  drivers/gpu/drm/arm/malidp_regs.h |  1 +
> >  drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
> >  drivers/gpu/drm/drm_writeback.c   | 30 +++++++++++++
> >  include/drm/drm_connector.h       |  4 ++
> >  include/drm/drm_mode_config.h     | 10 +++++
> >  11 files changed, 220 insertions(+), 62 deletions(-)
> >  create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h
> >
> > --
> > 2.7.4
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v3 0/2] Add writeback scaling

[Daniel Vetter]

On Tue, Apr 23, 2019 at 3:00 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>
> On Tue, Apr 23, 2019 at 02:24:11PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 23, 2019 at 12:48 PM Ben Davis <Ben.Davis@arm.com> wrote:
> > >
> > > Add support for scaling on writeback. To do this add writeback_dest_w
> > > and writeback_dest_h as writeback connector properties to specify the
> > > desired output dimensions.
> > > Then implement downscaling on writeback for Malidp-550 and Malidp-650
> > > (upscaling on writeback is not supported on these devices).
> > >
> > > v2: Use 0 as default for writeback_w,h and so update range to use 1 as
> > >     minimum.
> >
> > Hm I missed that, I don't think that's good, since it prevents
> > userspace from blindly writing back the properties it's read. We've
> > tried hard to avoid that, since we're suggesting compositor can take a
> > snapshot of all kms properties (including the ones they don't
> > understand) and restore that on vt switching. Hence stuff like fence
> > fds returning -1, and accepting -1 as NULL to make this work.
>
> Is that they way it should work, though? I remember a few good years
> back, before KMS, there were lots of issues with X server hanging and
> not restoring the vt mode correctly. Should it not be that on getting
> back control of a DRM master, an application sets it last known good
> mode that it had, before it lost (or gave away) control?

Given that we've recommended both approaches I expect there's a mix of
both approaches out there in the real world, with people just hacking
things up until it works. That's also what we're doing with the
in-kernel fbdev emulation. I'd say defacto this means it's all
ill-defined, but also that we imo should make a best effort to support
either approach.

> > tldr; I think range needs to include 0, and we need make that a
> > special thing, maybe enforced with a
> > drm_connector_state_compute_writeback_dst_h/w, which takes
> > crtc_state->mode.v/hdisplay into account if the writeback_dst_h/w is
> > 0.
>
> Repeating just to make sure we're on the same page: we allow 0 as valid
> range value, but writeback_dst_h/w ultimately gets updated before the
> commit gets passed on to the driver to use crtc_state->mode.v/hdisplay
> so as not to trigger any scaling. Correct?

That would also break stuff, since then you'd scale the next time
v/hdisplay are different. Except if we again clear them back to 0 on
duplicate_state, which I guess would work since writeback is one-shot.
I was thinking more of a helper function, but your approach +
resetting always to 0 on duplicate_state + lots of comments/kerneldoc
should work too. And I guess would be more robust, since driver
authors can't accidentally use the wrong state since it's fixed up.
-Daniel

>
> Best regards,
> Liviu
>
>
> > -Daniel
> >
> > >
> > > v3: Rename properties to specify they are destination width/height.
> > >     Make sure the values from the properties are passed to
> > >     enable_memwrite rather than the framebuffer dimensions
> > >
> > > Ben Davis (2):
> > >   drm: Add writeback_dest_w,h properties
> > >   drm/malidp: Enable writeback scaling
> > >
> > >  drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++---------
> > >  drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++
> > >  drivers/gpu/drm/arm/malidp_drv.c  | 10 +++--
> > >  drivers/gpu/drm/arm/malidp_hw.c   | 45 +++++++++++++------
> > >  drivers/gpu/drm/arm/malidp_hw.h   | 19 ++++++--
> > >  drivers/gpu/drm/arm/malidp_mw.c   | 94 ++++++++++++++++++++++++++++++---------
> > >  drivers/gpu/drm/arm/malidp_regs.h |  1 +
> > >  drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
> > >  drivers/gpu/drm/drm_writeback.c   | 30 +++++++++++++
> > >  include/drm/drm_connector.h       |  4 ++
> > >  include/drm/drm_mode_config.h     | 10 +++++
> > >  11 files changed, 220 insertions(+), 62 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h
> > >
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch