[PATCH v2 0/2] Add writeback scaling

[PATCH v2 0/2] Add writeback scaling

[Ben Davis]

Add support for scaling on writeback. To do this add writeback_w and
writeback_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.

Ben Davis (2):
  drm: Add writeback_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   | 86 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
 drivers/gpu/drm/drm_writeback.c   | 28 +++++++++++++
 include/drm/drm_connector.h       |  4 ++
 include/drm/drm_mode_config.h     | 10 +++++
 11 files changed, 211 insertions(+), 61 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h

-- 
2.7.4

[PATCH v2 0/2] Add writeback scaling

[Ben Davis]

Add support for scaling on writeback. To do this add writeback_w and
writeback_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.

Ben Davis (2):
  drm: Add writeback_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   | 86 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++
 drivers/gpu/drm/drm_writeback.c   | 28 +++++++++++++
 include/drm/drm_connector.h       |  4 ++
 include/drm/drm_mode_config.h     | 10 +++++
 11 files changed, 211 insertions(+), 61 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h

-- 
2.7.4

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

[PATCH v2 1/2] drm: Add writeback_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   | 28 ++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  4 ++++
 include/drm/drm_mode_config.h     | 10 ++++++++++
 4 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d520a04..1f68dce 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_w) {
+		state->writeback_w = val;
+	} else if (property == config->prop_writeback_h) {
+		state->writeback_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_w) {
+		*val = state->writeback_w;
+	} else if (property == config->prop_writeback_h) {
+		*val = state->writeback_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..3d0453e 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -74,6 +74,12 @@
  *	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_W":
+ *	The width of the writeback buffer to write back. 0 acts as default.
+ *
+ *  "WRITEBACK_H":
+ *	The height of the writeback buffer to write back. 0 acts as default.
  */
 
 #define fence_to_wb_connector(x) container_of(x->lock, \
@@ -141,6 +147,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_w) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_W", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_w = prop;
+	}
+
+	if (!dev->mode_config.prop_writeback_h) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_H", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_h = prop;
+	}
+
 	return 0;
 }
 
@@ -225,6 +247,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_w, 0);
+	drm_object_attach_property(&connector->base,
+				   config->prop_writeback_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..51c4cb2 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_w: width of plane to write to wb buffer */
+	/** @writeback_h: height of plane to write to wb buffer */
+	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_w;
+	/**
+	 * @prop_writeback_h: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_h;
+	/**
 	 * @prop_fb_id: Default atomic plane property to specify the
 	 * &drm_framebuffer.
 	 */
-- 
2.7.4

[PATCH v2 1/2] drm: Add writeback_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   | 28 ++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  4 ++++
 include/drm/drm_mode_config.h     | 10 ++++++++++
 4 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d520a04..1f68dce 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_w) {
+		state->writeback_w = val;
+	} else if (property == config->prop_writeback_h) {
+		state->writeback_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_w) {
+		*val = state->writeback_w;
+	} else if (property == config->prop_writeback_h) {
+		*val = state->writeback_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..3d0453e 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -74,6 +74,12 @@
  *	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_W":
+ *	The width of the writeback buffer to write back. 0 acts as default.
+ *
+ *  "WRITEBACK_H":
+ *	The height of the writeback buffer to write back. 0 acts as default.
  */
 
 #define fence_to_wb_connector(x) container_of(x->lock, \
@@ -141,6 +147,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_w) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_W", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_w = prop;
+	}
+
+	if (!dev->mode_config.prop_writeback_h) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_H", 1, UINT_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.prop_writeback_h = prop;
+	}
+
 	return 0;
 }
 
@@ -225,6 +247,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_w, 0);
+	drm_object_attach_property(&connector->base,
+				   config->prop_writeback_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..51c4cb2 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_w: width of plane to write to wb buffer */
+	/** @writeback_h: height of plane to write to wb buffer */
+	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_w;
+	/**
+	 * @prop_writeback_h: Writeback connector property for the plane
+	 * destination position in the writeback buffer.
+	 */
+	struct drm_property *prop_writeback_h;
+	/**
 	 * @prop_fb_id: Default atomic plane property to specify the
 	 * &drm_framebuffer.
 	 */
-- 
2.7.4

Re: [PATCH v2 1/2] drm: Add writeback_w,h properties

[Liviu Dudau]

Hi Ben,

On Fri, Apr 12, 2019 at 03:08:28PM +0100, Ben Davis wrote:
> 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   | 28 ++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  4 ++++
>  include/drm/drm_mode_config.h     | 10 ++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d520a04..1f68dce 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_w) {
> +		state->writeback_w = val;
> +	} else if (property == config->prop_writeback_h) {
> +		state->writeback_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_w) {
> +		*val = state->writeback_w;
> +	} else if (property == config->prop_writeback_h) {
> +		*val = state->writeback_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..3d0453e 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -74,6 +74,12 @@
>   *	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_W":
> + *	The width of the writeback buffer to write back. 0 acts as default.

To the "0 acts as default" I would add: "(i.e. its not set by userspace)".
I think it is worth explaining in more detail that 0 is default if no
value is set from userspace and that it also means no scaling.

Best regards,
Liviu

> + *
> + *  "WRITEBACK_H":
> + *	The height of the writeback buffer to write back. 0 acts as default.
>   */
>  
>  #define fence_to_wb_connector(x) container_of(x->lock, \
> @@ -141,6 +147,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_w) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_W", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_w = prop;
> +	}
> +
> +	if (!dev->mode_config.prop_writeback_h) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_H", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_h = prop;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -225,6 +247,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_w, 0);
> +	drm_object_attach_property(&connector->base,
> +				   config->prop_writeback_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..51c4cb2 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_w: width of plane to write to wb buffer */
> +	/** @writeback_h: height of plane to write to wb buffer */
> +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_w;
> +	/**
> +	 * @prop_writeback_h: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_h;
> +	/**
>  	 * @prop_fb_id: Default atomic plane property to specify the
>  	 * &drm_framebuffer.
>  	 */
> -- 
> 2.7.4
> 

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

Re: [PATCH v2 1/2] drm: Add writeback_w,h properties

[Liviu Dudau]

Hi Ben,

On Fri, Apr 12, 2019 at 03:08:28PM +0100, Ben Davis wrote:
> 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   | 28 ++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  4 ++++
>  include/drm/drm_mode_config.h     | 10 ++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d520a04..1f68dce 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_w) {
> +		state->writeback_w = val;
> +	} else if (property == config->prop_writeback_h) {
> +		state->writeback_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_w) {
> +		*val = state->writeback_w;
> +	} else if (property == config->prop_writeback_h) {
> +		*val = state->writeback_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..3d0453e 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -74,6 +74,12 @@
>   *	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_W":
> + *	The width of the writeback buffer to write back. 0 acts as default.

To the "0 acts as default" I would add: "(i.e. its not set by userspace)".
I think it is worth explaining in more detail that 0 is default if no
value is set from userspace and that it also means no scaling.

Best regards,
Liviu

> + *
> + *  "WRITEBACK_H":
> + *	The height of the writeback buffer to write back. 0 acts as default.
>   */
>  
>  #define fence_to_wb_connector(x) container_of(x->lock, \
> @@ -141,6 +147,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_w) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_W", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_w = prop;
> +	}
> +
> +	if (!dev->mode_config.prop_writeback_h) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_H", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_h = prop;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -225,6 +247,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_w, 0);
> +	drm_object_attach_property(&connector->base,
> +				   config->prop_writeback_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..51c4cb2 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_w: width of plane to write to wb buffer */
> +	/** @writeback_h: height of plane to write to wb buffer */
> +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_w;
> +	/**
> +	 * @prop_writeback_h: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_h;
> +	/**
>  	 * @prop_fb_id: Default atomic plane property to specify the
>  	 * &drm_framebuffer.
>  	 */
> -- 
> 2.7.4
> 

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

Re: [v2,1/2] drm: Add writeback_w,h properties

[james qian wang (Arm Technology China)]

On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> 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   | 28 ++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  4 ++++
>  include/drm/drm_mode_config.h     | 10 ++++++++++
>  4 files changed, 50 insertions(+)
> 
> -- 
> 2.7.4
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d520a04..1f68dce 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_w) {
> +		state->writeback_w = val;
> +	} else if (property == config->prop_writeback_h) {
> +		state->writeback_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_w) {
> +		*val = state->writeback_w;
> +	} else if (property == config->prop_writeback_h) {
> +		*val = state->writeback_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..3d0453e 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -74,6 +74,12 @@
>   *	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_W":
> + *	The width of the writeback buffer to write back. 0 acts as default.
> + *
> + *  "WRITEBACK_H":
> + *	The height of the writeback buffer to write back. 0 acts as default.
>   */
>  
>  #define fence_to_wb_connector(x) container_of(x->lock, \
> @@ -141,6 +147,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_w) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_W", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_w = prop;
> +	}
> +
> +	if (!dev->mode_config.prop_writeback_h) {
> +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +						 "WRITEBACK_H", 1, UINT_MAX);
> +		if (!prop)
> +			return -ENOMEM;
> +		dev->mode_config.prop_writeback_h = prop;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -225,6 +247,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_w, 0);
> +	drm_object_attach_property(&connector->base,
> +				   config->prop_writeback_h, 0);

Hi Ben:
Do we real need these two individual properties for specifing the writeback
w/h, can we use fb->w/h ?
And since these two properties are added as common and standard properties,
it influnce all existing write-back implementation which all assumed
writeback size as fb->w/h.
To compatible with existing writeback support, I suggest to keep to
use fb->w/h or add these properties as malidp private.

Thanks
James


>  	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..51c4cb2 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_w: width of plane to write to wb buffer */
> +	/** @writeback_h: height of plane to write to wb buffer */
> +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_w;
> +	/**
> +	 * @prop_writeback_h: Writeback connector property for the plane
> +	 * destination position in the writeback buffer.
> +	 */
> +	struct drm_property *prop_writeback_h;
> +	/**
>  	 * @prop_fb_id: Default atomic plane property to specify the
>  	 * &drm_framebuffer.
>  	 */

Pls del this window style line ending 

Re: [v2,1/2] drm: Add writeback_w,h properties

[Liviu Dudau]

On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > 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   | 28 ++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  4 ++++
> >  include/drm/drm_mode_config.h     | 10 ++++++++++
> >  4 files changed, 50 insertions(+)
> > 
> > -- 
> > 2.7.4
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d520a04..1f68dce 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_w) {
> > +		state->writeback_w = val;
> > +	} else if (property == config->prop_writeback_h) {
> > +		state->writeback_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_w) {
> > +		*val = state->writeback_w;
> > +	} else if (property == config->prop_writeback_h) {
> > +		*val = state->writeback_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..3d0453e 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -74,6 +74,12 @@
> >   *	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_W":
> > + *	The width of the writeback buffer to write back. 0 acts as default.
> > + *
> > + *  "WRITEBACK_H":
> > + *	The height of the writeback buffer to write back. 0 acts as default.
> >   */
> >  
> >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > @@ -141,6 +147,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_w) {
> > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +						 "WRITEBACK_W", 1, UINT_MAX);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		dev->mode_config.prop_writeback_w = prop;
> > +	}
> > +
> > +	if (!dev->mode_config.prop_writeback_h) {
> > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +						 "WRITEBACK_H", 1, UINT_MAX);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		dev->mode_config.prop_writeback_h = prop;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -225,6 +247,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_w, 0);
> > +	drm_object_attach_property(&connector->base,
> > +				   config->prop_writeback_h, 0);
> 
> Hi Ben:
> Do we real need these two individual properties for specifing the writeback
> w/h, can we use fb->w/h ?
> And since these two properties are added as common and standard properties,
> it influnce all existing write-back implementation which all assumed
> writeback size as fb->w/h.

The idea of having these additional properties was to maintain backwards
compatibility (of some sort). If you don't set writeback_w/h then the
assumption is that they are the same as fb->w/h, but I can see how it's
going to be confusing to have fb->w/h different from crtc->w/h and
different from writeback_w/h.


> To compatible with existing writeback support, I suggest to keep to
> use fb->w/h or add these properties as malidp private.

We don't need to make them malidp private, there is nothing malidp
specific in them. I'll talk with Ben, we should probably drop this patch
entirely and just enable malidp scaling when fb->w/h differ from
crtc->w/h.

Best regards,
Liviu


> 
> Thanks
> James
> 
> 
> >  	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..51c4cb2 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_w: width of plane to write to wb buffer */
> > +	/** @writeback_h: height of plane to write to wb buffer */
> > +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> > +	 * destination position in the writeback buffer.
> > +	 */
> > +	struct drm_property *prop_writeback_w;
> > +	/**
> > +	 * @prop_writeback_h: Writeback connector property for the plane
> > +	 * destination position in the writeback buffer.
> > +	 */
> > +	struct drm_property *prop_writeback_h;
> > +	/**
> >  	 * @prop_fb_id: Default atomic plane property to specify the
> >  	 * &drm_framebuffer.
> >  	 */
> 
> Pls del this window style line ending 
> 

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

Re: [v2,1/2] drm: Add writeback_w,h properties

[Daniel Vetter]

On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > > 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   | 28 ++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h       |  4 ++++
> > >  include/drm/drm_mode_config.h     | 10 ++++++++++
> > >  4 files changed, 50 insertions(+)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index d520a04..1f68dce 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_w) {
> > > +		state->writeback_w = val;
> > > +	} else if (property == config->prop_writeback_h) {
> > > +		state->writeback_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_w) {
> > > +		*val = state->writeback_w;
> > > +	} else if (property == config->prop_writeback_h) {
> > > +		*val = state->writeback_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..3d0453e 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -74,6 +74,12 @@
> > >   *	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_W":
> > > + *	The width of the writeback buffer to write back. 0 acts as default.
> > > + *
> > > + *  "WRITEBACK_H":
> > > + *	The height of the writeback buffer to write back. 0 acts as default.
> > >   */
> > >  
> > >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > > @@ -141,6 +147,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_w) {
> > > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > +						 "WRITEBACK_W", 1, UINT_MAX);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		dev->mode_config.prop_writeback_w = prop;
> > > +	}
> > > +
> > > +	if (!dev->mode_config.prop_writeback_h) {
> > > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > +						 "WRITEBACK_H", 1, UINT_MAX);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		dev->mode_config.prop_writeback_h = prop;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -225,6 +247,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_w, 0);
> > > +	drm_object_attach_property(&connector->base,
> > > +				   config->prop_writeback_h, 0);
> > 
> > Hi Ben:
> > Do we real need these two individual properties for specifing the writeback
> > w/h, can we use fb->w/h ?
> > And since these two properties are added as common and standard properties,
> > it influnce all existing write-back implementation which all assumed
> > writeback size as fb->w/h.
> 
> The idea of having these additional properties was to maintain backwards
> compatibility (of some sort). If you don't set writeback_w/h then the
> assumption is that they are the same as fb->w/h, but I can see how it's
> going to be confusing to have fb->w/h different from crtc->w/h and
> different from writeback_w/h.

Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
another set of w/h? Are all the interactions between these tree
well-defined?

Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
0, not scaled any further. But the planes themselves can be scaled into
the crtc_w/h window ofc.

Iirc the hw supporting writeback thus far needs a dedicated crtc for
writeback, so adding yet another scaling window is not needed?
-Daniel

> > To compatible with existing writeback support, I suggest to keep to
> > use fb->w/h or add these properties as malidp private.
> 
> We don't need to make them malidp private, there is nothing malidp
> specific in them. I'll talk with Ben, we should probably drop this patch
> entirely and just enable malidp scaling when fb->w/h differ from
> crtc->w/h.
> 
> Best regards,
> Liviu
> 
> 
> > 
> > Thanks
> > James
> > 
> > 
> > >  	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..51c4cb2 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_w: width of plane to write to wb buffer */
> > > +	/** @writeback_h: height of plane to write to wb buffer */
> > > +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> > > +	 * destination position in the writeback buffer.
> > > +	 */
> > > +	struct drm_property *prop_writeback_w;
> > > +	/**
> > > +	 * @prop_writeback_h: Writeback connector property for the plane
> > > +	 * destination position in the writeback buffer.
> > > +	 */
> > > +	struct drm_property *prop_writeback_h;
> > > +	/**
> > >  	 * @prop_fb_id: Default atomic plane property to specify the
> > >  	 * &drm_framebuffer.
> > >  	 */
> > 
> > Pls del this window style line ending 
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [v2,1/2] drm: Add writeback_w,h properties

[Liviu Dudau]

On Tue, Apr 16, 2019 at 09:34:20AM +0200, Daniel Vetter wrote:
> On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> > On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > > On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > > > 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   | 28 ++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h       |  4 ++++
> > > >  include/drm/drm_mode_config.h     | 10 ++++++++++
> > > >  4 files changed, 50 insertions(+)
> > > > 
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > index d520a04..1f68dce 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_w) {
> > > > +		state->writeback_w = val;
> > > > +	} else if (property == config->prop_writeback_h) {
> > > > +		state->writeback_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_w) {
> > > > +		*val = state->writeback_w;
> > > > +	} else if (property == config->prop_writeback_h) {
> > > > +		*val = state->writeback_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..3d0453e 100644
> > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > @@ -74,6 +74,12 @@
> > > >   *	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_W":
> > > > + *	The width of the writeback buffer to write back. 0 acts as default.
> > > > + *
> > > > + *  "WRITEBACK_H":
> > > > + *	The height of the writeback buffer to write back. 0 acts as default.
> > > >   */
> > > >  
> > > >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > > > @@ -141,6 +147,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_w) {
> > > > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > +						 "WRITEBACK_W", 1, UINT_MAX);
> > > > +		if (!prop)
> > > > +			return -ENOMEM;
> > > > +		dev->mode_config.prop_writeback_w = prop;
> > > > +	}
> > > > +
> > > > +	if (!dev->mode_config.prop_writeback_h) {
> > > > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > +						 "WRITEBACK_H", 1, UINT_MAX);
> > > > +		if (!prop)
> > > > +			return -ENOMEM;
> > > > +		dev->mode_config.prop_writeback_h = prop;
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -225,6 +247,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_w, 0);
> > > > +	drm_object_attach_property(&connector->base,
> > > > +				   config->prop_writeback_h, 0);
> > > 
> > > Hi Ben:
> > > Do we real need these two individual properties for specifing the writeback
> > > w/h, can we use fb->w/h ?
> > > And since these two properties are added as common and standard properties,
> > > it influnce all existing write-back implementation which all assumed
> > > writeback size as fb->w/h.
> > 
> > The idea of having these additional properties was to maintain backwards
> > compatibility (of some sort). If you don't set writeback_w/h then the
> > assumption is that they are the same as fb->w/h, but I can see how it's
> > going to be confusing to have fb->w/h different from crtc->w/h and
> > different from writeback_w/h.
> 
> Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
> another set of w/h? Are all the interactions between these tree
> well-defined?

No, we probably don't need another set of w/h values. As for the
interaction, I propose the following:

- writeback is only attached to a connector, so the crtc_x/y/w/h are the
  "input source" parameters into the writeback. Given that we put in the
  writeback the content of the CRTC, I suggest we ignore x,y (consider
  them to be always 0 for the writeback output).

- writeback has a framebuffer attached, so we can use the fb->w/h to
  determine if we need to do any scaling of the output.

> 
> Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
> 0, not scaled any further. But the planes themselves can be scaled into
> the crtc_w/h window ofc.

Our hardware has the ability to also write back one of the planes and 
scale it during writeback, but we have no way of expressing that in DRM
via connectors, so we are going to ignore that case. The supported usecase
is for the planes to be scaled into the "composition" area which is the size
of the CRTC, and then writeback that to memory, possibly with scaling (we
officially only going to support downscaling due to external dependencies
on the memory clock that is needed for supporting upscaling).

Best regards,

Liviu


> 
> Iirc the hw supporting writeback thus far needs a dedicated crtc for
> writeback, so adding yet another scaling window is not needed?
> -Daniel
> 
> > > To compatible with existing writeback support, I suggest to keep to
> > > use fb->w/h or add these properties as malidp private.
> > 
> > We don't need to make them malidp private, there is nothing malidp
> > specific in them. I'll talk with Ben, we should probably drop this patch
> > entirely and just enable malidp scaling when fb->w/h differ from
> > crtc->w/h.
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > Thanks
> > > James
> > > 
> > > 
> > > >  	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..51c4cb2 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_w: width of plane to write to wb buffer */
> > > > +	/** @writeback_h: height of plane to write to wb buffer */
> > > > +	uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> > > > +	 * destination position in the writeback buffer.
> > > > +	 */
> > > > +	struct drm_property *prop_writeback_w;
> > > > +	/**
> > > > +	 * @prop_writeback_h: Writeback connector property for the plane
> > > > +	 * destination position in the writeback buffer.
> > > > +	 */
> > > > +	struct drm_property *prop_writeback_h;
> > > > +	/**
> > > >  	 * @prop_fb_id: Default atomic plane property to specify the
> > > >  	 * &drm_framebuffer.
> > > >  	 */
> > > 
> > > Pls del this window style line ending 
> > > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [v2,1/2] drm: Add writeback_w,h properties

[Daniel Vetter]

On Tue, Apr 16, 2019 at 11:17 AM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>
> On Tue, Apr 16, 2019 at 09:34:20AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> > > On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > > > On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > > > > 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   | 28 ++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h       |  4 ++++
> > > > >  include/drm/drm_mode_config.h     | 10 ++++++++++
> > > > >  4 files changed, 50 insertions(+)
> > > > >
> > > > > --
> > > > > 2.7.4
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > index d520a04..1f68dce 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_w) {
> > > > > +               state->writeback_w = val;
> > > > > +       } else if (property == config->prop_writeback_h) {
> > > > > +               state->writeback_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_w) {
> > > > > +               *val = state->writeback_w;
> > > > > +       } else if (property == config->prop_writeback_h) {
> > > > > +               *val = state->writeback_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..3d0453e 100644
> > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > @@ -74,6 +74,12 @@
> > > > >   *     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_W":
> > > > > + *     The width of the writeback buffer to write back. 0 acts as default.
> > > > > + *
> > > > > + *  "WRITEBACK_H":
> > > > > + *     The height of the writeback buffer to write back. 0 acts as default.
> > > > >   */
> > > > >
> > > > >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > > > > @@ -141,6 +147,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_w) {
> > > > > +               prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > > +                                                "WRITEBACK_W", 1, UINT_MAX);
> > > > > +               if (!prop)
> > > > > +                       return -ENOMEM;
> > > > > +               dev->mode_config.prop_writeback_w = prop;
> > > > > +       }
> > > > > +
> > > > > +       if (!dev->mode_config.prop_writeback_h) {
> > > > > +               prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > > +                                                "WRITEBACK_H", 1, UINT_MAX);
> > > > > +               if (!prop)
> > > > > +                       return -ENOMEM;
> > > > > +               dev->mode_config.prop_writeback_h = prop;
> > > > > +       }
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > @@ -225,6 +247,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_w, 0);
> > > > > +       drm_object_attach_property(&connector->base,
> > > > > +                                  config->prop_writeback_h, 0);
> > > >
> > > > Hi Ben:
> > > > Do we real need these two individual properties for specifing the writeback
> > > > w/h, can we use fb->w/h ?
> > > > And since these two properties are added as common and standard properties,
> > > > it influnce all existing write-back implementation which all assumed
> > > > writeback size as fb->w/h.
> > >
> > > The idea of having these additional properties was to maintain backwards
> > > compatibility (of some sort). If you don't set writeback_w/h then the
> > > assumption is that they are the same as fb->w/h, but I can see how it's
> > > going to be confusing to have fb->w/h different from crtc->w/h and
> > > different from writeback_w/h.
> >
> > Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
> > another set of w/h? Are all the interactions between these tree
> > well-defined?
>
> No, we probably don't need another set of w/h values. As for the
> interaction, I propose the following:
>
> - writeback is only attached to a connector, so the crtc_x/y/w/h are the
>   "input source" parameters into the writeback. Given that we put in the
>   writeback the content of the CRTC, I suggest we ignore x,y (consider
>   them to be always 0 for the writeback output).

There's no crtc_x/y. And what I meant with crtc_w/h is
crtc_state->mode->h/vdisplay. And you can already express scaling with
that, by setting the plane_state->crtc_x/y/h/w parameters as you wish.

> - writeback has a framebuffer attached, so we can use the fb->w/h to
>   determine if we need to do any scaling of the output.

Hm, not sure we want that. You might want to automatically round
fb->w/h to whatever (ok right now we don't do that I think anywhere),
so forcing the output to match the fb->h/w doesn't make sense for me.
It's also inconsistent with how we treat fb attached to planes (where
we have the full set of src coordinates).

> > Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
> > 0, not scaled any further. But the planes themselves can be scaled into
> > the crtc_w/h window ofc.
>
> Our hardware has the ability to also write back one of the planes and
> scale it during writeback, but we have no way of expressing that in DRM
> via connectors, so we are going to ignore that case. The supported usecase
> is for the planes to be scaled into the "composition" area which is the size
> of the CRTC, and then writeback that to memory, possibly with scaling (we
> officially only going to support downscaling due to external dependencies
> on the memory clock that is needed for supporting upscaling).

Hm, see above, you can already express that:
- set crtc_state->mode->h/vdisplay to what you want the resulting
writeback image to be sized at.
- place the plane however you want using the plane properties within
that crtc window.

Cheers, Daniel

>
> Liviu
>
>
> >
> > Iirc the hw supporting writeback thus far needs a dedicated crtc for
> > writeback, so adding yet another scaling window is not needed?
> > -Daniel
> >
> > > > To compatible with existing writeback support, I suggest to keep to
> > > > use fb->w/h or add these properties as malidp private.
> > >
> > > We don't need to make them malidp private, there is nothing malidp
> > > specific in them. I'll talk with Ben, we should probably drop this patch
> > > entirely and just enable malidp scaling when fb->w/h differ from
> > > crtc->w/h.
> > >
> > > Best regards,
> > > Liviu
> > >
> > >
> > > >
> > > > Thanks
> > > > James
> > > >
> > > >
> > > > >         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..51c4cb2 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_w: width of plane to write to wb buffer */
> > > > > +       /** @writeback_h: height of plane to write to wb buffer */
> > > > > +       uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> > > > > +        * destination position in the writeback buffer.
> > > > > +        */
> > > > > +       struct drm_property *prop_writeback_w;
> > > > > +       /**
> > > > > +        * @prop_writeback_h: Writeback connector property for the plane
> > > > > +        * destination position in the writeback buffer.
> > > > > +        */
> > > > > +       struct drm_property *prop_writeback_h;
> > > > > +       /**
> > > > >          * @prop_fb_id: Default atomic plane property to specify the
> > > > >          * &drm_framebuffer.
> > > > >          */
> > > >
> > > > Pls del this window style line ending
> > > >
> > >
> > > --
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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

Re: [v2,1/2] drm: Add writeback_w,h properties

[Liviu Dudau]

On Tue, Apr 16, 2019 at 11:55:34AM +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 11:17 AM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> >
> > On Tue, Apr 16, 2019 at 09:34:20AM +0200, Daniel Vetter wrote:
> > > On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> > > > On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > > > > On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > > > > > 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   | 28 ++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_connector.h       |  4 ++++
> > > > > >  include/drm/drm_mode_config.h     | 10 ++++++++++
> > > > > >  4 files changed, 50 insertions(+)
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > > index d520a04..1f68dce 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_w) {
> > > > > > +               state->writeback_w = val;
> > > > > > +       } else if (property == config->prop_writeback_h) {
> > > > > > +               state->writeback_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_w) {
> > > > > > +               *val = state->writeback_w;
> > > > > > +       } else if (property == config->prop_writeback_h) {
> > > > > > +               *val = state->writeback_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..3d0453e 100644
> > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > @@ -74,6 +74,12 @@
> > > > > >   *     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_W":
> > > > > > + *     The width of the writeback buffer to write back. 0 acts as default.
> > > > > > + *
> > > > > > + *  "WRITEBACK_H":
> > > > > > + *     The height of the writeback buffer to write back. 0 acts as default.
> > > > > >   */
> > > > > >
> > > > > >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > > > > > @@ -141,6 +147,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_w) {
> > > > > > +               prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > > > +                                                "WRITEBACK_W", 1, UINT_MAX);
> > > > > > +               if (!prop)
> > > > > > +                       return -ENOMEM;
> > > > > > +               dev->mode_config.prop_writeback_w = prop;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (!dev->mode_config.prop_writeback_h) {
> > > > > > +               prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > > > > > +                                                "WRITEBACK_H", 1, UINT_MAX);
> > > > > > +               if (!prop)
> > > > > > +                       return -ENOMEM;
> > > > > > +               dev->mode_config.prop_writeback_h = prop;
> > > > > > +       }
> > > > > > +
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -225,6 +247,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_w, 0);
> > > > > > +       drm_object_attach_property(&connector->base,
> > > > > > +                                  config->prop_writeback_h, 0);
> > > > >
> > > > > Hi Ben:
> > > > > Do we real need these two individual properties for specifing the writeback
> > > > > w/h, can we use fb->w/h ?
> > > > > And since these two properties are added as common and standard properties,
> > > > > it influnce all existing write-back implementation which all assumed
> > > > > writeback size as fb->w/h.
> > > >
> > > > The idea of having these additional properties was to maintain backwards
> > > > compatibility (of some sort). If you don't set writeback_w/h then the
> > > > assumption is that they are the same as fb->w/h, but I can see how it's
> > > > going to be confusing to have fb->w/h different from crtc->w/h and
> > > > different from writeback_w/h.
> > >
> > > Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
> > > another set of w/h? Are all the interactions between these tree
> > > well-defined?
> >
> > No, we probably don't need another set of w/h values. As for the
> > interaction, I propose the following:
> >
> > - writeback is only attached to a connector, so the crtc_x/y/w/h are the
> >   "input source" parameters into the writeback. Given that we put in the
> >   writeback the content of the CRTC, I suggest we ignore x,y (consider
> >   them to be always 0 for the writeback output).
> 
> There's no crtc_x/y. 

Bah, sorry about that!

>                       And what I meant with crtc_w/h is
> crtc_state->mode->h/vdisplay. And you can already express scaling with
> that, by setting the plane_state->crtc_x/y/h/w parameters as you wish.

We don't have a plane associated with the writeback, it's a connector
with a framebuffer attached to it. :(

> 
> > - writeback has a framebuffer attached, so we can use the fb->w/h to
> >   determine if we need to do any scaling of the output.
> 
> Hm, not sure we want that. You might want to automatically round
> fb->w/h to whatever (ok right now we don't do that I think anywhere),
> so forcing the output to match the fb->h/w doesn't make sense for me.
> It's also inconsistent with how we treat fb attached to planes (where
> we have the full set of src coordinates).

The writeback will have to fit into that framebuffer, so it can't be
bigger than fb->w/h. Smaller, yes.


> 
> > > Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
> > > 0, not scaled any further. But the planes themselves can be scaled into
> > > the crtc_w/h window ofc.
> >
> > Our hardware has the ability to also write back one of the planes and
> > scale it during writeback, but we have no way of expressing that in DRM
> > via connectors, so we are going to ignore that case. The supported usecase
> > is for the planes to be scaled into the "composition" area which is the size
> > of the CRTC, and then writeback that to memory, possibly with scaling (we
> > officially only going to support downscaling due to external dependencies
> > on the memory clock that is needed for supporting upscaling).
> 
> Hm, see above, you can already express that:
> - set crtc_state->mode->h/vdisplay to what you want the resulting
> writeback image to be sized at.
> - place the plane however you want using the plane properties within
> that crtc window.

The way we agreed ~2 years ago in the community to implement this was to have
the writeback as a connector that gets the "output" of the CRTC and writes it into
memory rather than on phosphorus. We have not added a plane to that, but
in hindsight we might find it would have been useful.

Best regards,
Liviu


> 
> Cheers, Daniel
> 
> >
> > Liviu
> >
> >
> > >
> > > Iirc the hw supporting writeback thus far needs a dedicated crtc for
> > > writeback, so adding yet another scaling window is not needed?
> > > -Daniel
> > >
> > > > > To compatible with existing writeback support, I suggest to keep to
> > > > > use fb->w/h or add these properties as malidp private.
> > > >
> > > > We don't need to make them malidp private, there is nothing malidp
> > > > specific in them. I'll talk with Ben, we should probably drop this patch
> > > > entirely and just enable malidp scaling when fb->w/h differ from
> > > > crtc->w/h.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > > James
> > > > >
> > > > >
> > > > > >         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..51c4cb2 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_w: width of plane to write to wb buffer */
> > > > > > +       /** @writeback_h: height of plane to write to wb buffer */
> > > > > > +       uint32_t writeback_w, writeback_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..a0c2133 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_w: Writeback connector property for the plane
> > > > > > +        * destination position in the writeback buffer.
> > > > > > +        */
> > > > > > +       struct drm_property *prop_writeback_w;
> > > > > > +       /**
> > > > > > +        * @prop_writeback_h: Writeback connector property for the plane
> > > > > > +        * destination position in the writeback buffer.
> > > > > > +        */
> > > > > > +       struct drm_property *prop_writeback_h;
> > > > > > +       /**
> > > > > >          * @prop_fb_id: Default atomic plane property to specify the
> > > > > >          * &drm_framebuffer.
> > > > > >          */
> > > > >
> > > > > Pls del this window style line ending
> > > > >
> > > >
> > > > --
> > > > ====================
> > > > | I would like to |
> > > > | fix the world,  |
> > > > | but they're not |
> > > > | giving me the   |
> > > >  \ source code!  /
> > > >   ---------------
> > > >     ¯\_(ツ)_/¯
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> 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: [v2,1/2] drm: Add writeback_w,h properties

[Daniel Vetter]

On Tue, Apr 16, 2019 at 2:43 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>
> On Tue, Apr 16, 2019 at 11:55:34AM +0200, Daniel Vetter wrote:
> > On Tue, Apr 16, 2019 at 11:17 AM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > >
> > > On Tue, Apr 16, 2019 at 09:34:20AM +0200, Daniel Vetter wrote:
> > > > On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> > > > > On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > > > > > Hi Ben:
> > > > > > Do we real need these two individual properties for specifing the writeback
> > > > > > w/h, can we use fb->w/h ?
> > > > > > And since these two properties are added as common and standard properties,
> > > > > > it influnce all existing write-back implementation which all assumed
> > > > > > writeback size as fb->w/h.
> > > > >
> > > > > The idea of having these additional properties was to maintain backwards
> > > > > compatibility (of some sort). If you don't set writeback_w/h then the
> > > > > assumption is that they are the same as fb->w/h, but I can see how it's
> > > > > going to be confusing to have fb->w/h different from crtc->w/h and
> > > > > different from writeback_w/h.
> > > >
> > > > Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
> > > > another set of w/h? Are all the interactions between these tree
> > > > well-defined?
> > >
> > > No, we probably don't need another set of w/h values. As for the
> > > interaction, I propose the following:
> > >
> > > - writeback is only attached to a connector, so the crtc_x/y/w/h are the
> > >   "input source" parameters into the writeback. Given that we put in the
> > >   writeback the content of the CRTC, I suggest we ignore x,y (consider
> > >   them to be always 0 for the writeback output).
> >
> > There's no crtc_x/y.
>
> Bah, sorry about that!
>
> >                       And what I meant with crtc_w/h is
> > crtc_state->mode->h/vdisplay. And you can already express scaling with
> > that, by setting the plane_state->crtc_x/y/h/w parameters as you wish.
>
> We don't have a plane associated with the writeback, it's a connector
> with a framebuffer attached to it. :(

I meant the input planes, I know there's no output/writeback planes.
Afaiui what we want to do is:

planes -> plane composition hw -> some scaling -> writeback

We can model this already (I think at least, that's really my question
here) by adjustinv mode->h/vdisplay and ofc also adjusting dst_x/y/w/h
of all the input planes. But is that good enough?

With your proposal (either autoscaling per fb->w/h from the writeback
fb, or the writeback_w/h properties we'd have double-scaling:

planes -> plane composition hw -> some scaling (because of
crtc->mode.h/vdisplay) -> more scaling (due to writeback h/w, whether
as props or from the fb) -> writeback

Scaling twice with nothing else in the pipeline seems silly to me.

> > > - writeback has a framebuffer attached, so we can use the fb->w/h to
> > >   determine if we need to do any scaling of the output.
> >
> > Hm, not sure we want that. You might want to automatically round
> > fb->w/h to whatever (ok right now we don't do that I think anywhere),
> > so forcing the output to match the fb->h/w doesn't make sense for me.
> > It's also inconsistent with how we treat fb attached to planes (where
> > we have the full set of src coordinates).
>
> The writeback will have to fit into that framebuffer, so it can't be
> bigger than fb->w/h. Smaller, yes.

Yeah we should probably check that in core atomic code. Like we do
already for planes in drm_atomic_plane_check()

> > > > Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
> > > > 0, not scaled any further. But the planes themselves can be scaled into
> > > > the crtc_w/h window ofc.
> > >
> > > Our hardware has the ability to also write back one of the planes and
> > > scale it during writeback, but we have no way of expressing that in DRM
> > > via connectors, so we are going to ignore that case. The supported usecase
> > > is for the planes to be scaled into the "composition" area which is the size
> > > of the CRTC, and then writeback that to memory, possibly with scaling (we
> > > officially only going to support downscaling due to external dependencies
> > > on the memory clock that is needed for supporting upscaling).
> >
> > Hm, see above, you can already express that:
> > - set crtc_state->mode->h/vdisplay to what you want the resulting
> > writeback image to be sized at.
> > - place the plane however you want using the plane properties within
> > that crtc window.
>
> The way we agreed ~2 years ago in the community to implement this was to have
> the writeback as a connector that gets the "output" of the CRTC and writes it into
> memory rather than on phosphorus. We have not added a plane to that, but
> in hindsight we might find it would have been useful.

Still not talking about planes on the output side here :-) Adding a
plane would also just give you double-scaling (but with the benefit
that you can set writeback_x/y too, plus select a window of the
overall crtc to write back).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 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   | 86 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 7 files changed, 161 insertions(+), 61 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..2120ef1 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,76 @@ 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;
 
+
+	if ((conn_state->writeback_w != crtc_state->mode.hdisplay) ||
+	    (conn_state->writeback_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_w;
+		s->output_h = conn_state->writeback_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;
@@ -259,11 +309,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
 
 		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

[PATCH v2 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   | 86 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/arm/malidp_regs.h |  1 +
 7 files changed, 161 insertions(+), 61 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..2120ef1 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,76 @@ 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;
 
+
+	if ((conn_state->writeback_w != crtc_state->mode.hdisplay) ||
+	    (conn_state->writeback_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_w;
+		s->output_h = conn_state->writeback_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;
@@ -259,11 +309,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
 
 		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

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

Re: [PATCH v2 2/2] drm/malidp: Enable writeback scaling

[Liviu Dudau]

Hi Ben,

On Fri, Apr 12, 2019 at 03:08:32PM +0100, Ben Davis wrote:
> 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   | 86 ++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/arm/malidp_regs.h |  1 +
>  7 files changed, 161 insertions(+), 61 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..2120ef1 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,76 @@ 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;
>  
> +
> +	if ((conn_state->writeback_w != crtc_state->mode.hdisplay) ||
> +	    (conn_state->writeback_h != crtc_state->mode.vdisplay)) {

Now that the default (zero) for writeback_w and writeback_h means "not
set by userspace, don't scale" I think you need to revisit this check.
Otherwise the writeback scaling gets enabled all the time.

Best regards,
Liviu


> +		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_w;
> +		s->output_h = conn_state->writeback_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;
> @@ -259,11 +309,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
>  
>  		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
> 

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

Re: [PATCH v2 2/2] drm/malidp: Enable writeback scaling

[Liviu Dudau]

Hi Ben,

On Fri, Apr 12, 2019 at 03:08:32PM +0100, Ben Davis wrote:
> 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   | 86 ++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/arm/malidp_regs.h |  1 +
>  7 files changed, 161 insertions(+), 61 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..2120ef1 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,76 @@ 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;
>  
> +
> +	if ((conn_state->writeback_w != crtc_state->mode.hdisplay) ||
> +	    (conn_state->writeback_h != crtc_state->mode.vdisplay)) {

Now that the default (zero) for writeback_w and writeback_h means "not
set by userspace, don't scale" I think you need to revisit this check.
Otherwise the writeback scaling gets enabled all the time.

Best regards,
Liviu


> +		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_w;
> +		s->output_h = conn_state->writeback_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;
> @@ -259,11 +309,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
>  
>  		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
> 

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