[PATCH v2 0/5] R-Car DU: Fix IOMMU operation when connected to VSP

[PATCH v2 0/5] R-Car DU: Fix IOMMU operation when connected to VSP

[Laurent Pinchart]

Hello,

This patch series fixes the rcar-du-drm driver to support VSP plane sources
with an IOMMU. It is available for convenience at

        git://linuxtv.org/pinchartl/media.git iommu/devel/du

On R-Car Gen3 the DU has no direct memory access but sources planes through
VSP instances. When an IOMMU is inserted between the VSP and memory, the DU
framebuffers need to be DMA mapped using the VSP device, not the DU device as
currently done. The same situation can also be reproduced on Gen2 hardware by
linking the VSP to the DU in DT [1], effectively disabling direct memory
access by the DU.

The situation is made quite complex by the fact that different planes can be
connected to different DU instances, and thus served by different IOMMUs (or,
in practice on existing hardware, by the same IOMMU but through different
micro-TLBs). We thus can't allocate and map buffers to the right device in a
single dma_alloc_wc() operation as done in the DRM CMA GEM helpers.

However, on such setups, the DU DT node doesn't reference IOMMUs as the DU
does not perform any direct memory access. We can thus keep the GEM object
allocation unchanged, and the DMA addresses that we receive in the DU driver
will be physical addresses. Those buffers then need to be mapped to the VSP
device when they are associated with planes. Fortunately the atomic framework
provides two plane helper operations, .prepare_fb() and .cleanup_fb() that we
can use for this purpose.

The reality is slightly more complex than this on Gen3, as an FCP device
instance sits between VSP instances and memory. It is the FCP devices that are
connected to the IOMMUs, and buffer mapping thus need to be performed using
the FCP devices. This isn't required on Gen2 as the platforms don't have any
FCPs.

Patches 1/5 and 2/5 extend the rcar-fcp driver API to expose the FCP struct
device. Patch 3/5 then updates the vsp1 driver to map the display lists and
video buffers through the FCP when it exists. This alone fixes VSP operation
with an IOMMU on R-Car Gen3 systems.

Moving on to addressing the DU issue, patch 4/5 extends the vsp1 driver API to
allow mapping a scatter-gather list to the VSP, with the implementation using
the FCP devices instead when available. Patch 5/5 finally uses the vsp1
mapping API in the rcar-du-drm driver to map and unmap buffers when needed.

The series has been tested on the H2 Lager board and M3-W Salvator-X boards.
The IOMMU is known not to work properly on the H3 ES1.1, so the H3 Salvator-X
board hasn't been tested. In all cases both the DU and VSP operation has been
tested, and tests were run with and without linking the DU and VSP devices to
the IOMMU in DT.

For H2, the patches were tested on top of v4.12-rc1 with a set of out-of-tree
patches to link the VSP and DU to the IOMMUs and to enable VSP+DU combined
similar to R-Car Gen3, and an additional DMA mapping API patch [2] that fixes
IOMMU operation on ARM32, currently broken in v4.12-rc1. For M3-W, they were
were tested on top of renesas-drivers-2017-05-16-v4.12-rc1 with a set of
out-of-tree patches to add FCP, VSP, DU and IPMMU instances to the M3-W DT, as
well as a hack for the IPMMU driver to whitelist all bus master devices.

All tests passed successfully. The issue previously noticed on H3 with
synchronization between page flip and VSP operation that was caused by buffers
getting unmapped (and possibly freed) before the VSP was done reading them is
now gone thanks to the VSP+DU flicker fix that should be merged in v4.13 and
is available in renesas-drivers-2017-05-16-v4.12-rc1.

A possible improvement is to modify the GEM object allocation mechanism to use
non-contiguous memory when the DU driver detects that all the VSP instances it
is connected to use an IOMMU (possibly through FCP devices).

[1] https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg06589.html
[2] https://www.spinics.net/lists/arm-kernel/msg581410.html

Laurent Pinchart (4):
  v4l: rcar-fcp: Don't get/put module reference
  v4l: rcar-fcp: Add an API to retrieve the FCP device
  v4l: vsp1: Add API to map and unmap DRM buffers through the VSP
  drm: rcar-du: Map memory through the VSP device

Magnus Damm (1):
  v4l: vsp1: Map the DL and video buffers through the proper bus master

 drivers/gpu/drm/rcar-du/rcar_du_vsp.c    | 74 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h    |  2 +
 drivers/media/platform/rcar-fcp.c        | 17 ++++----
 drivers/media/platform/vsp1/vsp1.h       |  1 +
 drivers/media/platform/vsp1/vsp1_dl.c    |  4 +-
 drivers/media/platform/vsp1/vsp1_drm.c   | 25 +++++++++++
 drivers/media/platform/vsp1/vsp1_drv.c   |  9 ++++
 drivers/media/platform/vsp1/vsp1_video.c |  2 +-
 include/media/rcar-fcp.h                 |  5 +++
 include/media/vsp1.h                     |  3 ++
 10 files changed, 124 insertions(+), 18 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/5] v4l: rcar-fcp: Don't get/put module reference

[Laurent Pinchart]

Direct callers of the FCP API hold a reference to the FCP module due to
module linkage, there's no need to take another one manually. Take a
reference to the device instead to ensure that it won't disappear behind
the caller's back.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-fcp.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/rcar-fcp.c b/drivers/media/platform/rcar-fcp.c
index 7146fc5ef168..e9f609edf513 100644
--- a/drivers/media/platform/rcar-fcp.c
+++ b/drivers/media/platform/rcar-fcp.c
@@ -53,14 +53,7 @@ struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
 		if (fcp->dev->of_node != np)
 			continue;
 
-		/*
-		 * Make sure the module won't be unloaded behind our back. This
-		 * is a poor man's safety net, the module should really not be
-		 * unloaded while FCP users can be active.
-		 */
-		if (!try_module_get(fcp->dev->driver->owner))
-			fcp = NULL;
-
+		get_device(fcp->dev);
 		goto done;
 	}
 
@@ -81,7 +74,7 @@ EXPORT_SYMBOL_GPL(rcar_fcp_get);
 void rcar_fcp_put(struct rcar_fcp_device *fcp)
 {
 	if (fcp)
-		module_put(fcp->dev->driver->owner);
+		put_device(fcp->dev);
 }
 EXPORT_SYMBOL_GPL(rcar_fcp_put);
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/5] v4l: rcar-fcp: Don't get/put module reference

[Laurent Pinchart]

Direct callers of the FCP API hold a reference to the FCP module due to
module linkage, there's no need to take another one manually. Take a
reference to the device instead to ensure that it won't disappear behind
the caller's back.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-fcp.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/rcar-fcp.c b/drivers/media/platform/rcar-fcp.c
index 7146fc5ef168..e9f609edf513 100644
--- a/drivers/media/platform/rcar-fcp.c
+++ b/drivers/media/platform/rcar-fcp.c
@@ -53,14 +53,7 @@ struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
 		if (fcp->dev->of_node != np)
 			continue;
 
-		/*
-		 * Make sure the module won't be unloaded behind our back. This
-		 * is a poor man's safety net, the module should really not be
-		 * unloaded while FCP users can be active.
-		 */
-		if (!try_module_get(fcp->dev->driver->owner))
-			fcp = NULL;
-
+		get_device(fcp->dev);
 		goto done;
 	}
 
@@ -81,7 +74,7 @@ EXPORT_SYMBOL_GPL(rcar_fcp_get);
 void rcar_fcp_put(struct rcar_fcp_device *fcp)
 {
 	if (fcp)
-		module_put(fcp->dev->driver->owner);
+		put_device(fcp->dev);
 }
 EXPORT_SYMBOL_GPL(rcar_fcp_put);
 
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/5] v4l: rcar-fcp: Don't get/put module reference

[Kieran Bingham]

Hi Laurent,

Thanks for the patch:

On 17/05/17 00:20, Laurent Pinchart wrote:
> Direct callers of the FCP API hold a reference to the FCP module due to
> module linkage, there's no need to take another one manually. Take a
> reference to the device instead to ensure that it won't disappear behind
> the caller's back.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-fcp.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-fcp.c b/drivers/media/platform/rcar-fcp.c
> index 7146fc5ef168..e9f609edf513 100644
> --- a/drivers/media/platform/rcar-fcp.c
> +++ b/drivers/media/platform/rcar-fcp.c
> @@ -53,14 +53,7 @@ struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
>  		if (fcp->dev->of_node != np)
>  			continue;
>  
> -		/*
> -		 * Make sure the module won't be unloaded behind our back. This
> -		 * is a poor man's safety net, the module should really not be
> -		 * unloaded while FCP users can be active.
> -		 */
> -		if (!try_module_get(fcp->dev->driver->owner))
> -			fcp = NULL;
> -
> +		get_device(fcp->dev);
>  		goto done;
>  	}
>  
> @@ -81,7 +74,7 @@ EXPORT_SYMBOL_GPL(rcar_fcp_get);
>  void rcar_fcp_put(struct rcar_fcp_device *fcp)
>  {
>  	if (fcp)
> -		module_put(fcp->dev->driver->owner);
> +		put_device(fcp->dev);
>  }
>  EXPORT_SYMBOL_GPL(rcar_fcp_put);
>  
> 

[PATCH v2 2/5] v4l: rcar-fcp: Add an API to retrieve the FCP device

[Laurent Pinchart]

The new rcar_fcp_get_device() function retrieves the struct device
related to the FCP device. This is useful to handle DMA mapping through
the right device.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-fcp.c | 6 ++++++
 include/media/rcar-fcp.h          | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/media/platform/rcar-fcp.c b/drivers/media/platform/rcar-fcp.c
index e9f609edf513..2988031d285d 100644
--- a/drivers/media/platform/rcar-fcp.c
+++ b/drivers/media/platform/rcar-fcp.c
@@ -78,6 +78,12 @@ void rcar_fcp_put(struct rcar_fcp_device *fcp)
 }
 EXPORT_SYMBOL_GPL(rcar_fcp_put);
 
+struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp)
+{
+	return fcp->dev;
+}
+EXPORT_SYMBOL_GPL(rcar_fcp_get_device);
+
 /**
  * rcar_fcp_enable - Enable an FCP
  * @fcp: The FCP instance
diff --git a/include/media/rcar-fcp.h b/include/media/rcar-fcp.h
index 8723f05c6321..b60a7b176c37 100644
--- a/include/media/rcar-fcp.h
+++ b/include/media/rcar-fcp.h
@@ -19,6 +19,7 @@ struct rcar_fcp_device;
 #if IS_ENABLED(CONFIG_VIDEO_RENESAS_FCP)
 struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np);
 void rcar_fcp_put(struct rcar_fcp_device *fcp);
+struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp);
 int rcar_fcp_enable(struct rcar_fcp_device *fcp);
 void rcar_fcp_disable(struct rcar_fcp_device *fcp);
 #else
@@ -27,6 +28,10 @@ static inline struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
 	return ERR_PTR(-ENOENT);
 }
 static inline void rcar_fcp_put(struct rcar_fcp_device *fcp) { }
+static inline struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp)
+{
+	return NULL;
+}
 static inline int rcar_fcp_enable(struct rcar_fcp_device *fcp)
 {
 	return 0;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] v4l: rcar-fcp: Add an API to retrieve the FCP device

[Kieran Bingham]

Hi Laurent,

Thankyou for the patch:

On 17/05/17 00:20, Laurent Pinchart wrote:
> The new rcar_fcp_get_device() function retrieves the struct device
> related to the FCP device. This is useful to handle DMA mapping through
> the right device.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-fcp.c | 6 ++++++
>  include/media/rcar-fcp.h          | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-fcp.c b/drivers/media/platform/rcar-fcp.c
> index e9f609edf513..2988031d285d 100644
> --- a/drivers/media/platform/rcar-fcp.c
> +++ b/drivers/media/platform/rcar-fcp.c
> @@ -78,6 +78,12 @@ void rcar_fcp_put(struct rcar_fcp_device *fcp)
>  }
>  EXPORT_SYMBOL_GPL(rcar_fcp_put);
>  
> +struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp)
> +{
> +	return fcp->dev;
> +}
> +EXPORT_SYMBOL_GPL(rcar_fcp_get_device);
> +
>  /**
>   * rcar_fcp_enable - Enable an FCP
>   * @fcp: The FCP instance
> diff --git a/include/media/rcar-fcp.h b/include/media/rcar-fcp.h
> index 8723f05c6321..b60a7b176c37 100644
> --- a/include/media/rcar-fcp.h
> +++ b/include/media/rcar-fcp.h
> @@ -19,6 +19,7 @@ struct rcar_fcp_device;
>  #if IS_ENABLED(CONFIG_VIDEO_RENESAS_FCP)
>  struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np);
>  void rcar_fcp_put(struct rcar_fcp_device *fcp);
> +struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp);
>  int rcar_fcp_enable(struct rcar_fcp_device *fcp);
>  void rcar_fcp_disable(struct rcar_fcp_device *fcp);
>  #else
> @@ -27,6 +28,10 @@ static inline struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
>  	return ERR_PTR(-ENOENT);
>  }
>  static inline void rcar_fcp_put(struct rcar_fcp_device *fcp) { }
> +static inline struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp)
> +{
> +	return NULL;
> +}
>  static inline int rcar_fcp_enable(struct rcar_fcp_device *fcp)
>  {
>  	return 0;
> 

[PATCH v2 3/5] v4l: vsp1: Map the DL and video buffers through the proper bus master

[Laurent Pinchart]

From: Magnus Damm <magnus.damm@gmail.com>

On Gen2 hardware the VSP1 is a bus master and accesses the display list
and video buffers through DMA directly. On Gen3 hardware, however,
memory accesses go through a separate IP core called FCP.

The VSP1 driver unconditionally maps DMA buffers through the VSP device.
While this doesn't cause any practical issue so far, DMA mappings will
be incorrect as soon as we will enable IOMMU support for the FCP on Gen3
platforms, resulting in IOMMU faults.

Fix this by mapping all buffers through the FCP device if present, and
through the VSP1 device as usual otherwise.

Suggested-by: Magnus Damm <magnus.damm@gmail.com>
[Cache the bus master device]
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1.h       | 1 +
 drivers/media/platform/vsp1/vsp1_dl.c    | 4 ++--
 drivers/media/platform/vsp1/vsp1_drv.c   | 9 +++++++++
 drivers/media/platform/vsp1/vsp1_video.c | 2 +-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h
index 85387a64179a..847963b6e9eb 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -74,6 +74,7 @@ struct vsp1_device {
 
 	void __iomem *mmio;
 	struct rcar_fcp_device *fcp;
+	struct device *bus_master;
 
 	struct vsp1_bru *bru;
 	struct vsp1_clu *clu;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 7d8f37772b56..445d1c31fff3 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -137,7 +137,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
 	dlb->vsp1 = vsp1;
 	dlb->size = size;
 
-	dlb->entries = dma_alloc_wc(vsp1->dev, dlb->size, &dlb->dma,
+	dlb->entries = dma_alloc_wc(vsp1->bus_master, dlb->size, &dlb->dma,
 				    GFP_KERNEL);
 	if (!dlb->entries)
 		return -ENOMEM;
@@ -150,7 +150,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
  */
 static void vsp1_dl_body_cleanup(struct vsp1_dl_body *dlb)
 {
-	dma_free_wc(dlb->vsp1->dev, dlb->size, dlb->entries, dlb->dma);
+	dma_free_wc(dlb->vsp1->bus_master, dlb->size, dlb->entries, dlb->dma);
 }
 
 /**
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 048446af5ae7..95c26edead85 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -764,6 +764,15 @@ static int vsp1_probe(struct platform_device *pdev)
 				PTR_ERR(vsp1->fcp));
 			return PTR_ERR(vsp1->fcp);
 		}
+
+		/*
+		 * When the FCP is present, it handles all bus master accesses
+		 * for the VSP and must thus be used in place of the VSP device
+		 * to map DMA buffers.
+		 */
+		vsp1->bus_master = rcar_fcp_get_device(vsp1->fcp);
+	} else {
+		vsp1->bus_master = vsp1->dev;
 	}
 
 	/* Configure device parameters based on the version register. */
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index eab3c3ea85d7..5af3486afe07 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -1197,7 +1197,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
 	video->queue.ops = &vsp1_video_queue_qops;
 	video->queue.mem_ops = &vb2_dma_contig_memops;
 	video->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
-	video->queue.dev = video->vsp1->dev;
+	video->queue.dev = video->vsp1->bus_master;
 	ret = vb2_queue_init(&video->queue);
 	if (ret < 0) {
 		dev_err(video->vsp1->dev, "failed to initialize vb2 queue\n");
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/5] v4l: vsp1: Map the DL and video buffers through the proper bus master

[Kieran Bingham]

Hi Laurent,

Thanks for the patch:

On 17/05/17 00:20, Laurent Pinchart wrote:
> From: Magnus Damm <magnus.damm@gmail.com>
> 
> On Gen2 hardware the VSP1 is a bus master and accesses the display list
> and video buffers through DMA directly. On Gen3 hardware, however,
> memory accesses go through a separate IP core called FCP.
> 
> The VSP1 driver unconditionally maps DMA buffers through the VSP device.
> While this doesn't cause any practical issue so far, DMA mappings will
> be incorrect as soon as we will enable IOMMU support for the FCP on Gen3
> platforms, resulting in IOMMU faults.
> 
> Fix this by mapping all buffers through the FCP device if present, and
> through the VSP1 device as usual otherwise.
> 
> Suggested-by: Magnus Damm <magnus.damm@gmail.com>
> [Cache the bus master device]
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/media/platform/vsp1/vsp1.h       | 1 +
>  drivers/media/platform/vsp1/vsp1_dl.c    | 4 ++--
>  drivers/media/platform/vsp1/vsp1_drv.c   | 9 +++++++++
>  drivers/media/platform/vsp1/vsp1_video.c | 2 +-
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h
> index 85387a64179a..847963b6e9eb 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -74,6 +74,7 @@ struct vsp1_device {
>  
>  	void __iomem *mmio;
>  	struct rcar_fcp_device *fcp;
> +	struct device *bus_master;
>  
>  	struct vsp1_bru *bru;
>  	struct vsp1_clu *clu;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index 7d8f37772b56..445d1c31fff3 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -137,7 +137,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
>  	dlb->vsp1 = vsp1;
>  	dlb->size = size;
>  
> -	dlb->entries = dma_alloc_wc(vsp1->dev, dlb->size, &dlb->dma,
> +	dlb->entries = dma_alloc_wc(vsp1->bus_master, dlb->size, &dlb->dma,
>  				    GFP_KERNEL);
>  	if (!dlb->entries)
>  		return -ENOMEM;
> @@ -150,7 +150,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
>   */
>  static void vsp1_dl_body_cleanup(struct vsp1_dl_body *dlb)
>  {
> -	dma_free_wc(dlb->vsp1->dev, dlb->size, dlb->entries, dlb->dma);
> +	dma_free_wc(dlb->vsp1->bus_master, dlb->size, dlb->entries, dlb->dma);
>  }
>  
>  /**
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index 048446af5ae7..95c26edead85 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -764,6 +764,15 @@ static int vsp1_probe(struct platform_device *pdev)
>  				PTR_ERR(vsp1->fcp));
>  			return PTR_ERR(vsp1->fcp);
>  		}
> +
> +		/*
> +		 * When the FCP is present, it handles all bus master accesses
> +		 * for the VSP and must thus be used in place of the VSP device
> +		 * to map DMA buffers.
> +		 */
> +		vsp1->bus_master = rcar_fcp_get_device(vsp1->fcp);
> +	} else {
> +		vsp1->bus_master = vsp1->dev;
>  	}
>  
>  	/* Configure device parameters based on the version register. */
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index eab3c3ea85d7..5af3486afe07 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -1197,7 +1197,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
>  	video->queue.ops = &vsp1_video_queue_qops;
>  	video->queue.mem_ops = &vb2_dma_contig_memops;
>  	video->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> -	video->queue.dev = video->vsp1->dev;
> +	video->queue.dev = video->vsp1->bus_master;
>  	ret = vb2_queue_init(&video->queue);
>  	if (ret < 0) {
>  		dev_err(video->vsp1->dev, "failed to initialize vb2 queue\n");
> 

[PATCH v2 4/5] v4l: vsp1: Add API to map and unmap DRM buffers through the VSP

[Laurent Pinchart]

The display buffers must be mapped for DMA through the device that
performs memory access. Expose an API to map and unmap memory through
the VSP device to be used by the DU.

As all the buffers allocated by the DU driver are coherent, we can skip
cache handling when mapping and unmapping them. This will need to be
revisited when support for non-coherent buffers will be added to the DU
driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 25 +++++++++++++++++++++++++
 include/media/vsp1.h                   |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 9d235e830f5a..c809c2aadca0 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -12,9 +12,11 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/slab.h>
 
 #include <media/media-entity.h>
+#include <media/rcar-fcp.h>
 #include <media/v4l2-subdev.h>
 #include <media/vsp1.h>
 
@@ -524,6 +526,29 @@ void vsp1_du_atomic_flush(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
 
+int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt)
+{
+	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+
+	/*
+	 * As all the buffers allocated by the DU driver are coherent, we can
+	 * skip cache sync. This will need to be revisited when support for
+	 * non-coherent buffers will be added to the DU driver.
+	 */
+	return dma_map_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
+				DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+}
+EXPORT_SYMBOL_GPL(vsp1_du_map_sg);
+
+void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
+{
+	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+
+	dma_unmap_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
+			   DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+}
+EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
+
 /* -----------------------------------------------------------------------------
  * Initialization
  */
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 38aac554dbba..6aa630c9f7af 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -13,6 +13,7 @@
 #ifndef __MEDIA_VSP1_H__
 #define __MEDIA_VSP1_H__
 
+#include <linux/scatterlist.h>
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
@@ -46,5 +47,7 @@ void vsp1_du_atomic_begin(struct device *dev);
 int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
 			  const struct vsp1_du_atomic_config *cfg);
 void vsp1_du_atomic_flush(struct device *dev);
+int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
+void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
 
 #endif /* __MEDIA_VSP1_H__ */
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] v4l: vsp1: Add API to map and unmap DRM buffers through the VSP

[Kieran Bingham]

Hi Laurent,

On 17/05/17 00:20, Laurent Pinchart wrote:
> The display buffers must be mapped for DMA through the device that
> performs memory access. Expose an API to map and unmap memory through
> the VSP device to be used by the DU.
> 
> As all the buffers allocated by the DU driver are coherent, we can skip
> cache handling when mapping and unmapping them. This will need to be
> revisited when support for non-coherent buffers will be added to the DU
> driver.

Thankyou for the patch

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

With the small nitpick fixed:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 25 +++++++++++++++++++++++++
>  include/media/vsp1.h                   |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 9d235e830f5a..c809c2aadca0 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -12,9 +12,11 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/slab.h>
>  
>  #include <media/media-entity.h>
> +#include <media/rcar-fcp.h>

This header isn't used here.
(Presumably it was before you cached the fcp dev as the vsp1->bus_master).

I'll fix this while applying locally. No need to resend.

>  #include <media/v4l2-subdev.h>
>  #include <media/vsp1.h>
>  
> @@ -524,6 +526,29 @@ void vsp1_du_atomic_flush(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
>  
> +int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> +	/*
> +	 * As all the buffers allocated by the DU driver are coherent, we can
> +	 * skip cache sync. This will need to be revisited when support for
> +	 * non-coherent buffers will be added to the DU driver.
> +	 */
> +	return dma_map_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
> +				DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_map_sg);
> +
> +void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> +	dma_unmap_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
> +			   DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
> +
>  /* -----------------------------------------------------------------------------
>   * Initialization
>   */
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 38aac554dbba..6aa630c9f7af 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -13,6 +13,7 @@
>  #ifndef __MEDIA_VSP1_H__
>  #define __MEDIA_VSP1_H__
>  
> +#include <linux/scatterlist.h>
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
>  
> @@ -46,5 +47,7 @@ void vsp1_du_atomic_begin(struct device *dev);
>  int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
>  			  const struct vsp1_du_atomic_config *cfg);
>  void vsp1_du_atomic_flush(struct device *dev);
> +int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
> +void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
>  
>  #endif /* __MEDIA_VSP1_H__ */
> 

[PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Laurent Pinchart]

For planes handled by a VSP instance, map the framebuffer memory through
the VSP to ensure proper IOMMU handling.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |  2 +
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index b0ff304ce3dc..1b874cfd40f3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -19,7 +19,9 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
 
+#include <linux/dma-mapping.h>
 #include <linux/of_platform.h>
+#include <linux/scatterlist.h>
 #include <linux/videodev2.h>
 
 #include <media/vsp1.h>
@@ -170,12 +172,9 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
 	cfg.dst.width = state->state.crtc_w;
 	cfg.dst.height = state->state.crtc_h;
 
-	for (i = 0; i < state->format->planes; ++i) {
-		struct drm_gem_cma_object *gem;
-
-		gem = drm_fb_cma_get_gem_obj(fb, i);
-		cfg.mem[i] = gem->paddr + fb->offsets[i];
-	}
+	for (i = 0; i < state->format->planes; ++i)
+		cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
+			   + fb->offsets[i];
 
 	for (i = 0; i < ARRAY_SIZE(formats_kms); ++i) {
 		if (formats_kms[i] == state->format->fourcc) {
@@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
 	vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
 }
 
+static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
+					struct drm_plane_state *state)
+{
+	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
+	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
+	struct rcar_du_device *rcdu = vsp->dev;
+	unsigned int i;
+	int ret;
+
+	if (!state->fb)
+		return 0;
+
+	for (i = 0; i < rstate->format->planes; ++i) {
+		struct drm_gem_cma_object *gem =
+			drm_fb_cma_get_gem_obj(state->fb, i);
+		struct sg_table *sgt = &rstate->sg_tables[i];
+
+		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
+				      gem->base.size);
+		if (ret)
+			goto fail;
+
+		ret = vsp1_du_map_sg(vsp->vsp, sgt);
+		if (!ret) {
+			sg_free_table(sgt);
+			ret = -ENOMEM;
+			goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	for (i--; i >= 0; i--) {
+		struct sg_table *sgt = &rstate->sg_tables[i];
+
+		vsp1_du_unmap_sg(vsp->vsp, sgt);
+		sg_free_table(sgt);
+	}
+
+	return ret;
+}
+
+static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
+					 struct drm_plane_state *state)
+{
+	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
+	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
+	unsigned int i;
+
+	if (!state->fb)
+		return;
+
+	for (i = 0; i < rstate->format->planes; ++i) {
+		struct sg_table *sgt = &rstate->sg_tables[i];
+
+		vsp1_du_unmap_sg(vsp->vsp, sgt);
+		sg_free_table(sgt);
+	}
+}
+
 static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
 					  struct drm_plane_state *state)
 {
@@ -227,6 +287,8 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = {
+	.prepare_fb = rcar_du_vsp_plane_prepare_fb,
+	.cleanup_fb = rcar_du_vsp_plane_cleanup_fb,
 	.atomic_check = rcar_du_vsp_plane_atomic_check,
 	.atomic_update = rcar_du_vsp_plane_atomic_update,
 };
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
index f1d0f1824528..8861661590ff 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
@@ -43,6 +43,7 @@ static inline struct rcar_du_vsp_plane *to_rcar_vsp_plane(struct drm_plane *p)
  * struct rcar_du_vsp_plane_state - Driver-specific plane state
  * @state: base DRM plane state
  * @format: information about the pixel format used by the plane
+ * @sg_tables: scatter-gather tables for the frame buffer memory
  * @alpha: value of the plane alpha property
  * @zpos: value of the plane zpos property
  */
@@ -50,6 +51,7 @@ struct rcar_du_vsp_plane_state {
 	struct drm_plane_state state;
 
 	const struct rcar_du_format_info *format;
+	struct sg_table sg_tables[3];
 
 	unsigned int alpha;
 	unsigned int zpos;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Kieran Bingham]

Hi Laurent,

Thankyou for the patch.

On 17/05/17 00:20, Laurent Pinchart wrote:
> For planes handled by a VSP instance, map the framebuffer memory through
> the VSP to ensure proper IOMMU handling.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Looks good except for a small unsigned int comparison causing an infinite loop
on fail case.

With the loop fixed:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 ++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |  2 +
>  2 files changed, 70 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index b0ff304ce3dc..1b874cfd40f3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -19,7 +19,9 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
>  
> +#include <linux/dma-mapping.h>
>  #include <linux/of_platform.h>
> +#include <linux/scatterlist.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/vsp1.h>
> @@ -170,12 +172,9 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  	cfg.dst.width = state->state.crtc_w;
>  	cfg.dst.height = state->state.crtc_h;
>  
> -	for (i = 0; i < state->format->planes; ++i) {
> -		struct drm_gem_cma_object *gem;
> -
> -		gem = drm_fb_cma_get_gem_obj(fb, i);
> -		cfg.mem[i] = gem->paddr + fb->offsets[i];
> -	}
> +	for (i = 0; i < state->format->planes; ++i)
> +		cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
> +			   + fb->offsets[i];
>  
>  	for (i = 0; i < ARRAY_SIZE(formats_kms); ++i) {
>  		if (formats_kms[i] == state->format->fourcc) {
> @@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  	vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
>  }
>  
> +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> +					struct drm_plane_state *state)
> +{
> +	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> +	struct rcar_du_device *rcdu = vsp->dev;
> +	unsigned int i;

Unsigned here..

> +	int ret;
> +
> +	if (!state->fb)
> +		return 0;
> +
> +	for (i = 0; i < rstate->format->planes; ++i) {
> +		struct drm_gem_cma_object *gem =
> +			drm_fb_cma_get_gem_obj(state->fb, i);
> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
> +				      gem->base.size);
> +		if (ret)
> +			goto fail;
> +
> +		ret = vsp1_du_map_sg(vsp->vsp, sgt);
> +		if (!ret) {
> +			sg_free_table(sgt);
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +fail:
> +	for (i--; i >= 0; i--) {

Means that this loop will never exit; i will always be >= 0;

I'd propose just making signed for this usage.

I've checked the i values for the breakouts - so they are good, and we need to
use i == 0 to unmap the last table.

> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> +		sg_free_table(sgt);
> +	}
> +
> +	return ret;
> +}
> +
> +static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
> +					 struct drm_plane_state *state)
> +{
> +	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> +	unsigned int i;
> +
> +	if (!state->fb)
> +		return;
> +
> +	for (i = 0; i < rstate->format->planes; ++i) {
> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> +		sg_free_table(sgt);
> +	}
> +}
> +
>  static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
>  					  struct drm_plane_state *state)
>  {
> @@ -227,6 +287,8 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane,
>  }
>  
>  static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = {
> +	.prepare_fb = rcar_du_vsp_plane_prepare_fb,
> +	.cleanup_fb = rcar_du_vsp_plane_cleanup_fb,
>  	.atomic_check = rcar_du_vsp_plane_atomic_check,
>  	.atomic_update = rcar_du_vsp_plane_atomic_update,
>  };
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> index f1d0f1824528..8861661590ff 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> @@ -43,6 +43,7 @@ static inline struct rcar_du_vsp_plane *to_rcar_vsp_plane(struct drm_plane *p)
>   * struct rcar_du_vsp_plane_state - Driver-specific plane state
>   * @state: base DRM plane state
>   * @format: information about the pixel format used by the plane
> + * @sg_tables: scatter-gather tables for the frame buffer memory
>   * @alpha: value of the plane alpha property
>   * @zpos: value of the plane zpos property
>   */
> @@ -50,6 +51,7 @@ struct rcar_du_vsp_plane_state {
>  	struct drm_plane_state state;
>  
>  	const struct rcar_du_format_info *format;
> +	struct sg_table sg_tables[3];
>  
>  	unsigned int alpha;
>  	unsigned int zpos;
> 

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Kieran Bingham]

Hi Laurent,

Thankyou for the patch.

On 17/05/17 00:20, Laurent Pinchart wrote:
> For planes handled by a VSP instance, map the framebuffer memory through
> the VSP to ensure proper IOMMU handling.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Looks good except for a small unsigned int comparison causing an infinite loop
on fail case.

With the loop fixed:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 ++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |  2 +
>  2 files changed, 70 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index b0ff304ce3dc..1b874cfd40f3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -19,7 +19,9 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
>  
> +#include <linux/dma-mapping.h>
>  #include <linux/of_platform.h>
> +#include <linux/scatterlist.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/vsp1.h>
> @@ -170,12 +172,9 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  	cfg.dst.width = state->state.crtc_w;
>  	cfg.dst.height = state->state.crtc_h;
>  
> -	for (i = 0; i < state->format->planes; ++i) {
> -		struct drm_gem_cma_object *gem;
> -
> -		gem = drm_fb_cma_get_gem_obj(fb, i);
> -		cfg.mem[i] = gem->paddr + fb->offsets[i];
> -	}
> +	for (i = 0; i < state->format->planes; ++i)
> +		cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
> +			   + fb->offsets[i];
>  
>  	for (i = 0; i < ARRAY_SIZE(formats_kms); ++i) {
>  		if (formats_kms[i] == state->format->fourcc) {
> @@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  	vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
>  }
>  
> +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> +					struct drm_plane_state *state)
> +{
> +	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> +	struct rcar_du_device *rcdu = vsp->dev;
> +	unsigned int i;

Unsigned here..

> +	int ret;
> +
> +	if (!state->fb)
> +		return 0;
> +
> +	for (i = 0; i < rstate->format->planes; ++i) {
> +		struct drm_gem_cma_object *gem =
> +			drm_fb_cma_get_gem_obj(state->fb, i);
> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
> +				      gem->base.size);
> +		if (ret)
> +			goto fail;
> +
> +		ret = vsp1_du_map_sg(vsp->vsp, sgt);
> +		if (!ret) {
> +			sg_free_table(sgt);
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +fail:
> +	for (i--; i >= 0; i--) {

Means that this loop will never exit; i will always be >= 0;

I'd propose just making signed for this usage.

I've checked the i values for the breakouts - so they are good, and we need to
use i == 0 to unmap the last table.

> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> +		sg_free_table(sgt);
> +	}
> +
> +	return ret;
> +}
> +
> +static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
> +					 struct drm_plane_state *state)
> +{
> +	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> +	unsigned int i;
> +
> +	if (!state->fb)
> +		return;
> +
> +	for (i = 0; i < rstate->format->planes; ++i) {
> +		struct sg_table *sgt = &rstate->sg_tables[i];
> +
> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> +		sg_free_table(sgt);
> +	}
> +}
> +
>  static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
>  					  struct drm_plane_state *state)
>  {
> @@ -227,6 +287,8 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane,
>  }
>  
>  static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = {
> +	.prepare_fb = rcar_du_vsp_plane_prepare_fb,
> +	.cleanup_fb = rcar_du_vsp_plane_cleanup_fb,
>  	.atomic_check = rcar_du_vsp_plane_atomic_check,
>  	.atomic_update = rcar_du_vsp_plane_atomic_update,
>  };
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> index f1d0f1824528..8861661590ff 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> @@ -43,6 +43,7 @@ static inline struct rcar_du_vsp_plane *to_rcar_vsp_plane(struct drm_plane *p)
>   * struct rcar_du_vsp_plane_state - Driver-specific plane state
>   * @state: base DRM plane state
>   * @format: information about the pixel format used by the plane
> + * @sg_tables: scatter-gather tables for the frame buffer memory
>   * @alpha: value of the plane alpha property
>   * @zpos: value of the plane zpos property
>   */
> @@ -50,6 +51,7 @@ struct rcar_du_vsp_plane_state {
>  	struct drm_plane_state state;
>  
>  	const struct rcar_du_format_info *format;
> +	struct sg_table sg_tables[3];
>  
>  	unsigned int alpha;
>  	unsigned int zpos;
> 

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Laurent Pinchart]

Hi Kieran,

On Monday 22 May 2017 13:16:11 Kieran Bingham wrote:
> On 17/05/17 00:20, Laurent Pinchart wrote:
> > For planes handled by a VSP instance, map the framebuffer memory through
> > the VSP to ensure proper IOMMU handling.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Looks good except for a small unsigned int comparison causing an infinite
> loop on fail case.
> 
> With the loop fixed:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 +++++++++++++++++++++++++++---
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |  2 +
> >  2 files changed, 70 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1b874cfd40f3
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c

[snip]


> > @@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct
> > rcar_du_vsp_plane *plane)
> >  	vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
> >  }
> > 
> > +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> > +					struct drm_plane_state *state)
> > +{
> > +	struct rcar_du_vsp_plane_state *rstate = 
to_rcar_vsp_plane_state(state);
> > +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> > +	struct rcar_du_device *rcdu = vsp->dev;
> > +	unsigned int i;
> 
> Unsigned here..
> 
> > +	int ret;
> > +
> > +	if (!state->fb)
> > +		return 0;
> > +
> > +	for (i = 0; i < rstate->format->planes; ++i) {
> > +		struct drm_gem_cma_object *gem =
> > +			drm_fb_cma_get_gem_obj(state->fb, i);
> > +		struct sg_table *sgt = &rstate->sg_tables[i];
> > +
> > +		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
> > +				      gem->base.size);
> > +		if (ret)
> > +			goto fail;
> > +
> > +		ret = vsp1_du_map_sg(vsp->vsp, sgt);
> > +		if (!ret) {
> > +			sg_free_table(sgt);
> > +			ret = -ENOMEM;
> > +			goto fail;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +fail:
> > +	for (i--; i >= 0; i--) {
> 
> Means that this loop will never exit; i will always be >= 0;
> 
> I'd propose just making signed for this usage.
> 
> I've checked the i values for the breakouts - so they are good, and we need
> to use i == 0 to unmap the last table.
> 
> > +		struct sg_table *sgt = &rstate->sg_tables[i];

How about keep i unsigned and using

	for (; i > 0; i--) {
		struct sg_table *sgt = &rstate->sg_tables[i-1];
		...
	}

If you want to fix while applying, you can pick your favourite version.

> > +
> > +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> > +		sg_free_table(sgt);
> > +	}
> > +
> > +	return ret;
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Kieran Bingham]

Hi Laurent,

On 22/05/17 13:24, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Monday 22 May 2017 13:16:11 Kieran Bingham wrote:
>> On 17/05/17 00:20, Laurent Pinchart wrote:
>>> For planes handled by a VSP instance, map the framebuffer memory through
>>> the VSP to ensure proper IOMMU handling.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>
>> Looks good except for a small unsigned int comparison causing an infinite
>> loop on fail case.
>>
>> With the loop fixed:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>>> ---
>>>
>>>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 +++++++++++++++++++++++++++---
>>>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |  2 +
>>>  2 files changed, 70 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1b874cfd40f3
>>> 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> 
> [snip]
> 
> 
>>> @@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct
>>> rcar_du_vsp_plane *plane)
>>>  	vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
>>>  }
>>>
>>> +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
>>> +					struct drm_plane_state *state)
>>> +{
>>> +	struct rcar_du_vsp_plane_state *rstate = 
> to_rcar_vsp_plane_state(state);
>>> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
>>> +	struct rcar_du_device *rcdu = vsp->dev;
>>> +	unsigned int i;
>>
>> Unsigned here..
>>
>>> +	int ret;
>>> +
>>> +	if (!state->fb)
>>> +		return 0;
>>> +
>>> +	for (i = 0; i < rstate->format->planes; ++i) {
>>> +		struct drm_gem_cma_object *gem =
>>> +			drm_fb_cma_get_gem_obj(state->fb, i);
>>> +		struct sg_table *sgt = &rstate->sg_tables[i];
>>> +
>>> +		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
>>> +				      gem->base.size);
>>> +		if (ret)
>>> +			goto fail;
>>> +
>>> +		ret = vsp1_du_map_sg(vsp->vsp, sgt);
>>> +		if (!ret) {
>>> +			sg_free_table(sgt);
>>> +			ret = -ENOMEM;
>>> +			goto fail;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +fail:
>>> +	for (i--; i >= 0; i--) {
>>
>> Means that this loop will never exit; i will always be >= 0;
>>
>> I'd propose just making signed for this usage.
>>
>> I've checked the i values for the breakouts - so they are good, and we need
>> to use i == 0 to unmap the last table.
>>
>>> +		struct sg_table *sgt = &rstate->sg_tables[i];
> 
> How about keep i unsigned and using
> 
> 	for (; i > 0; i--) {
> 		struct sg_table *sgt = &rstate->sg_tables[i-1];
> 		...
> 	}

My only distaste there is having to then add the [i-1] index to the sg_tables.

I have just experimented with:

fail:
        for (; i-- != 0;) {
                struct sg_table *sgt = &rstate->sg_tables[i];
		...
	}

This performs the correct loops, with the correct indexes, but does the
decrement in the condition offend coding styles ?

If that's disliked even more I'll just apply your suggestion.

--
Kieran


> 
> If you want to fix while applying, you can pick your favourite version.
> 
>>> +
>>> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
>>> +		sg_free_table(sgt);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
> 

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Geert Uytterhoeven]

On Mon, May 22, 2017 at 2:52 PM, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> My only distaste there is having to then add the [i-1] index to the sg_tables.
>
> I have just experimented with:
>
> fail:
>         for (; i-- != 0;) {
>                 struct sg_table *sgt = &rstate->sg_tables[i];
>                 ...
>         }
>
> This performs the correct loops, with the correct indexes, but does the
> decrement in the condition offend coding styles ?
>
> If that's disliked even more I'll just apply your suggestion.

You can still use "i-- > 0", which looks a little bit better IMHO.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Laurent Pinchart]

Hello Geert and Kieran,

On Monday 22 May 2017 15:00:27 Geert Uytterhoeven wrote:
> On Mon, May 22, 2017 at 2:52 PM, Kieran Bingham wrote:
> > My only distaste there is having to then add the [i-1] index to the
> > sg_tables.
> > 
> > I have just experimented with:
> > 
> > fail:
> >         for (; i-- != 0;) {
> >                 struct sg_table *sgt = &rstate->sg_tables[i];
> >                 ...
> >         }
> > 
> > This performs the correct loops, with the correct indexes, but does the
> > decrement in the condition offend coding styles ?
> > 
> > If that's disliked even more I'll just apply your suggestion.
> 
> You can still use "i-- > 0", which looks a little bit better IMHO.

I'm fine with that option too.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] drm: rcar-du: Map memory through the VSP device

[Kieran Bingham]

On 22/05/17 14:23, Laurent Pinchart wrote:
> Hello Geert and Kieran,
> 
> On Monday 22 May 2017 15:00:27 Geert Uytterhoeven wrote:
>> On Mon, May 22, 2017 at 2:52 PM, Kieran Bingham wrote:
>>> My only distaste there is having to then add the [i-1] index to the
>>> sg_tables.
>>>
>>> I have just experimented with:
>>>
>>> fail:
>>>         for (; i-- != 0;) {
>>>                 struct sg_table *sgt = &rstate->sg_tables[i];
>>>                 ...
>>>         }
>>>
>>> This performs the correct loops, with the correct indexes, but does the
>>> decrement in the condition offend coding styles ?
>>>
>>> If that's disliked even more I'll just apply your suggestion.
>>
>> You can still use "i-- > 0", which looks a little bit better IMHO.
> 
> I'm fine with that option too.
> 

Of course for(; X ;) is just while(X), which is also more readable ;)

And while (i-- > 0) simplifies cleanly to while (i--) which I'm sure is quite
readable.

I'll clean up and post the updated series including linux-media.

--

Kieran