Re: [PATCH 00/14] staging: media: tegra-vdea: Add Tegra124 support

[Thierry Reding]

On Mon, Sep 03, 2018 at 02:18:15PM +0200, Hans Verkuil wrote:
> Hi Thierry, Dmitry,
> 
> Dmitry found some issues, so I'll wait for a v2.
> 
> Anyway, this driver is in staging with this TODO:
> 
> - Implement V4L2 API once it gains support for stateless decoders.
> 
> I just wanted to mention that the Request API is expected to be merged
> for 4.20. A topic branch is here:
> 
> https://git.linuxtv.org/media_tree.git/log/?h=request_api
> 
> This patch series is expected to be added to the topic branch once
> everyone agrees:
> 
> https://www.spinics.net/lists/linux-media/msg139713.html
> 
> The first Allwinner driver that will be using this API is here:
> 
> https://lwn.net/Articles/763589/
> 
> It's expected to be merged for 4.20 as well.
> 
> Preliminary H264 work for the Allwinner driver is here:
> 
> https://lkml.org/lkml/2018/6/13/399
> 
> But this needs more work.
> 
> HEVC support, on the other hand, is almost ready:
> 
> https://lkml.org/lkml/2018/8/28/229
> 
> I hope these links give a good overview of the current status.

Thanks for those links. I was aware of the ongoing efforts and was
eagerly waiting for the various pieces to settle a bit. I will hopefully
get around to porting the tegra-vde driver to this new infrastructure in
the next couple of weeks.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 01/14] staging: media: tegra-vde: Support BSEV clock and reset

[Thierry Reding]

On Mon, Aug 13, 2018 at 06:09:46PM +0300, Dmitry Osipenko wrote:
> On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote:
> > From: Thierry Reding 
> > 
> > The BSEV clock has a separate gate bit and can not be assumed to be
> > always enabled. Add explicit handling for the BSEV clock and reset.
> > 
> > This fixes an issue on Tegra124 where the BSEV clock is not enabled
> > by default and therefore accessing the BSEV registers will hang the
> > CPU if the BSEV clock is not enabled and the reset not deasserted.
> > 
> > Signed-off-by: Thierry Reding 
> > ---
> 
> Are you sure that BSEV clock is really needed for T20/30? I've tried already 
> to disable the clock explicitly and everything kept working, though I'll try 
> again.

I think you're right that these aren't strictly required for VDE to work
on Tegra20 and Tegra30. However, the BSEV clock and reset do exist on
those platforms, so I didn't see a reason why they shouldn't be handled
uniformly across all generations.

> The device-tree changes should be reflected in the binding documentation.

Indeed, I forgot to update that.

Thierry


signature.asc
Description: PGP signature

[PATCH 14/14] ARM: tegra: Enable SMMU for VDE on Tegra124

[Thierry Reding]

From: Thierry Reding 

The video decode engine can use the SMMU to use buffers that are not
physically contiguous in memory. This allows better memory usage for
video decoding, since fragmentation may cause contiguous allocations
to fail.

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra124.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 8fdca4723205..0713e0ed5fef 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -321,6 +321,8 @@
resets = <_car 61>,
 <_car 63>;
reset-names = "vde", "bsev";
+
+   iommus = < TEGRA_SWGROUP_VDE>;
};
 
apbdma: dma@6002 {
-- 
2.17.0

[PATCH 12/14] ARM: tegra: Add BSEV clock and reset for VDE on Tegra20

[Thierry Reding]

From: Thierry Reding 

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra20.dtsi | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 15b73bd377f0..abb5738a0705 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -287,9 +287,13 @@
 , /* BSE-V 
interrupt */
 ; /* SXE interrupt 
*/
interrupt-names = "sync-token", "bsev", "sxe";
-   clocks = <_car TEGRA20_CLK_VDE>;
-   reset-names = "vde", "mc";
-   resets = <_car 61>, < TEGRA20_MC_RESET_VDE>;
+   clocks = <_car TEGRA20_CLK_VDE>,
+<_car TEGRA20_CLK_BSEV>;
+   clock-names = "vde", "bsev";
+   resets = <_car 61>,
+<_car 63>,
+< TEGRA20_MC_RESET_VDE>;
+   reset-names = "vde", "bsev", "mc";
};
 
apbmisc@7800 {
-- 
2.17.0

[PATCH 13/14] ARM: tegra: Add BSEV clock and reset for VDE on Tegra30

[Thierry Reding]

From: Thierry Reding 

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra30.dtsi | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index a6781f653310..492917d61bab 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -408,9 +408,13 @@
 , /* BSE-V 
interrupt */
 ; /* SXE interrupt 
*/
interrupt-names = "sync-token", "bsev", "sxe";
-   clocks = <_car TEGRA30_CLK_VDE>;
-   reset-names = "vde", "mc";
-   resets = <_car 61>, < TEGRA30_MC_RESET_VDE>;
+   clocks = <_car TEGRA30_CLK_VDE>,
+<_car TEGRA30_CLK_BSEV>;
+   clock-names = "vde", "bsev";
+   resets = <_car 61>,
+<_car 63>,
+< TEGRA30_MC_RESET_VDE>;
+   reset-names = "vde", "bsev", "mc";
};
 
apbmisc@7800 {
-- 
2.17.0

[PATCH 08/14] staging: media: tegra-vde: Track struct device *

[Thierry Reding]

From: Thierry Reding 

The pointer to the struct device is frequently used, so store it in
struct tegra_vde. Also, pass around a pointer to a struct tegra_vde
instead of struct device in some cases to prepare for subsequent
patches referencing additional data from that structure.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 63 -
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 41cf86dc5dbd..2496a03fd158 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -71,6 +71,7 @@ struct tegra_vde_soc {
 };
 
 struct tegra_vde {
+   struct device *dev;
const struct tegra_vde_soc *soc;
void __iomem *sxe;
void __iomem *bsev;
@@ -644,7 +645,7 @@ static void tegra_vde_detach_and_put_dmabuf(struct 
dma_buf_attachment *a,
dma_buf_put(dmabuf);
 }
 
-static int tegra_vde_attach_dmabuf(struct device *dev,
+static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
   int fd,
   unsigned long offset,
   size_t min_size,
@@ -662,38 +663,40 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf)) {
-   dev_err(dev, "Invalid dmabuf FD: %d\n", fd);
+   dev_err(vde->dev, "Invalid dmabuf FD: %d\n", fd);
return PTR_ERR(dmabuf);
}
 
if (dmabuf->size & (align_size - 1)) {
-   dev_err(dev, "Unaligned dmabuf 0x%zX, should be aligned to 
0x%zX\n",
+   dev_err(vde->dev,
+   "Unaligned dmabuf 0x%zX, should be aligned to 0x%zX\n",
dmabuf->size, align_size);
return -EINVAL;
}
 
if ((u64)offset + min_size > dmabuf->size) {
-   dev_err(dev, "Too small dmabuf size %zu @0x%lX, should be at 
least %zu\n",
+   dev_err(vde->dev,
+   "Too small dmabuf size %zu @0x%lX, should be at least 
%zu\n",
dmabuf->size, offset, min_size);
return -EINVAL;
}
 
-   attachment = dma_buf_attach(dmabuf, dev);
+   attachment = dma_buf_attach(dmabuf, vde->dev);
if (IS_ERR(attachment)) {
-   dev_err(dev, "Failed to attach dmabuf\n");
+   dev_err(vde->dev, "Failed to attach dmabuf\n");
err = PTR_ERR(attachment);
goto err_put;
}
 
sgt = dma_buf_map_attachment(attachment, dma_dir);
if (IS_ERR(sgt)) {
-   dev_err(dev, "Failed to get dmabufs sg_table\n");
+   dev_err(vde->dev, "Failed to get dmabufs sg_table\n");
err = PTR_ERR(sgt);
goto err_detach;
}
 
if (sgt->nents != 1) {
-   dev_err(dev, "Sparse DMA region is unsupported\n");
+   dev_err(vde->dev, "Sparse DMA region is unsupported\n");
err = -EINVAL;
goto err_unmap;
}
@@ -717,7 +720,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
return err;
 }
 
-static int tegra_vde_attach_dmabufs_to_frame(struct device *dev,
+static int tegra_vde_attach_dmabufs_to_frame(struct tegra_vde *vde,
 struct video_frame *frame,
 struct tegra_vde_h264_frame *src,
 enum dma_data_direction dma_dir,
@@ -726,7 +729,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device 
*dev,
 {
int err;
 
-   err = tegra_vde_attach_dmabuf(dev, src->y_fd,
+   err = tegra_vde_attach_dmabuf(vde, src->y_fd,
  src->y_offset, lsize, SZ_256,
  >y_dmabuf_attachment,
  >y_addr,
@@ -735,7 +738,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device 
*dev,
if (err)
return err;
 
-   err = tegra_vde_attach_dmabuf(dev, src->cb_fd,
+   err = tegra_vde_attach_dmabuf(vde, src->cb_fd,
  src->cb_offset, csize, SZ_256,
  >cb_dmabuf_attachment,
  >cb_addr,
@@ -744,7 +747,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device 
*dev,
if (err)
goto err_release_y;
 
-   err = tegra_vde_attach_dmabuf(dev, src->cr_fd,
+   err = tegra_vde_attach_dmabuf(vde, src->cr_fd,
  src->cr_offset, csize,

[PATCH 10/14] staging: media: tegra-vde: Keep VDE in reset when unused

[Thierry Reding]

From: Thierry Reding 

There is no point in keeping the VDE module out of reset when it is not
in use. Reset it on runtime suspend.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 3bc0bfcfe34e..4b3c6ab3c77e 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -1226,6 +1226,7 @@ static int tegra_vde_runtime_suspend(struct device *dev)
}
 
reset_control_assert(vde->rst_bsev);
+   reset_control_assert(vde->rst);
 
usleep_range(2000, 4000);
 
-- 
2.17.0

[PATCH 11/14] ARM: tegra: Enable VDE on Tegra124

[Thierry Reding]

From: Thierry Reding 

Signed-off-by: Thierry Reding 
---
 arch/arm/boot/dts/tegra124.dtsi | 40 +
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index b113e47b2b2a..8fdca4723205 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -83,6 +83,19 @@
};
};
 
+   iram@4000 {
+   compatible = "mmio-sram";
+   reg = <0x0 0x4000 0x0 0x4>;
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 0x0 0x4000 0x4>;
+
+   vde_pool: pool@400 {
+   reg = <0x400 0x3fc00>;
+   pool;
+   };
+   };
+
host1x@5000 {
compatible = "nvidia,tegra124-host1x", "simple-bus";
reg = <0x0 0x5000 0x0 0x00034000>;
@@ -283,6 +296,33 @@
*/
};
 
+   vde@6003 {
+   compatible = "nvidia,tegra124-vde", "nvidia,tegra30-vde",
+"nvidia,tegra20-vde";
+   reg = <0x0 0x6003 0x0 0x1000   /* Syntax Engine */
+  0x0 0x60031000 0x0 0x1000   /* Video Bitstream Engine */
+  0x0 0x60032000 0x0 0x0100   /* Macroblock Engine */
+  0x0 0x60032200 0x0 0x0100   /* Post-processing Engine */
+  0x0 0x60032400 0x0 0x0100   /* Motion Compensation 
Engine */
+  0x0 0x60032600 0x0 0x0100   /* Transform Engine */
+  0x0 0x60032800 0x0 0x0100   /* Pixel prediction block */
+  0x0 0x60032a00 0x0 0x0100   /* Video DMA */
+  0x0 0x60033800 0x0 0x0400>; /* Video frame controls */
+   reg-names = "sxe", "bsev", "mbe", "ppe", "mce",
+   "tfe", "ppb", "vdma", "frameid";
+   iram = <_pool>; /* IRAM region */
+   interrupts = , /* Sync token 
interrupt */
+, /* BSE-V 
interrupt */
+; /* SXE interrupt 
*/
+   interrupt-names = "sync-token", "bsev", "sxe";
+   clocks = <_car TEGRA124_CLK_VDE>,
+<_car TEGRA124_CLK_BSEV>;
+   clock-names = "vde", "bsev";
+   resets = <_car 61>,
+<_car 63>;
+   reset-names = "vde", "bsev";
+   };
+
apbdma: dma@6002 {
compatible = "nvidia,tegra124-apbdma", "nvidia,tegra148-apbdma";
reg = <0x0 0x6002 0x0 0x1400>;
-- 
2.17.0

[PATCH 09/14] staging: media: tegra-vde: Add IOMMU support

[Thierry Reding]

From: Thierry Reding 

Implement support for using an IOMMU to map physically discontiguous
buffers into contiguous I/O virtual mappings that the VDE can use. This
allows importing arbitrary DMA-BUFs for use by the VDE.

While at it, make sure that the device is detached from any DMA/IOMMU
mapping that it might have automatically been attached to at boot. If
using the IOMMU API explicitly, detaching from any existing mapping is
required to avoid double mapping of buffers.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 171 +---
 1 file changed, 153 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 2496a03fd158..3bc0bfcfe34e 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -13,7 +13,9 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -22,6 +24,10 @@
 #include 
 #include 
 
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
+#include 
+#endif
+
 #include 
 
 #include 
@@ -61,6 +67,11 @@ struct video_frame {
u32 frame_num;
u32 flags;
u64 modifier;
+
+   struct iova *y_iova;
+   struct iova *cb_iova;
+   struct iova *cr_iova;
+   struct iova *aux_iova;
 };
 
 struct tegra_vde_soc {
@@ -93,6 +104,12 @@ struct tegra_vde {
struct clk *clk_bsev;
dma_addr_t iram_lists_addr;
u32 *iram;
+
+   struct iommu_domain *domain;
+   struct iommu_group *group;
+   struct iova_domain iova;
+   unsigned long limit;
+   unsigned int shift;
 };
 
 static void tegra_vde_set_bits(struct tegra_vde *vde,
@@ -634,12 +651,22 @@ static void tegra_vde_decode_frame(struct tegra_vde *vde,
VDE_WR(0x2000 | (macroblocks_nb - 1), vde->sxe + 0x00);
 }
 
-static void tegra_vde_detach_and_put_dmabuf(struct dma_buf_attachment *a,
+static void tegra_vde_detach_and_put_dmabuf(struct tegra_vde *vde,
+   struct dma_buf_attachment *a,
struct sg_table *sgt,
+   struct iova *iova,
enum dma_data_direction dma_dir)
 {
struct dma_buf *dmabuf = a->dmabuf;
 
+   if (vde->domain) {
+   unsigned long size = iova_size(iova) << vde->shift;
+   dma_addr_t addr = iova_dma_addr(>iova, iova);
+
+   iommu_unmap(vde->domain, addr, size);
+   __free_iova(>iova, iova);
+   }
+
dma_buf_unmap_attachment(a, sgt, dma_dir);
dma_buf_detach(dmabuf, a);
dma_buf_put(dmabuf);
@@ -651,14 +678,16 @@ static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
   size_t min_size,
   size_t align_size,
   struct dma_buf_attachment **a,
-  dma_addr_t *addr,
+  dma_addr_t *addrp,
   struct sg_table **s,
-  size_t *size,
+  struct iova **iovap,
+  size_t *sizep,
   enum dma_data_direction dma_dir)
 {
struct dma_buf_attachment *attachment;
struct dma_buf *dmabuf;
struct sg_table *sgt;
+   size_t size;
int err;
 
dmabuf = dma_buf_get(fd);
@@ -695,18 +724,47 @@ static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
goto err_detach;
}
 
-   if (sgt->nents != 1) {
+   if (sgt->nents > 1 && !vde->domain) {
dev_err(vde->dev, "Sparse DMA region is unsupported\n");
err = -EINVAL;
goto err_unmap;
}
 
-   *addr = sg_dma_address(sgt->sgl) + offset;
+   if (vde->domain) {
+   int prot = IOMMU_READ | IOMMU_WRITE;
+   struct iova *iova;
+   dma_addr_t addr;
+
+   size = (dmabuf->size - offset) >> vde->shift;
+
+   iova = alloc_iova(>iova, size, vde->limit - 1, true);
+   if (!iova) {
+   err = -ENOMEM;
+   goto err_unmap;
+   }
+
+   addr = iova_dma_addr(>iova, iova);
+
+   size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
+   prot);
+   if (!size) {
+   __free_iova(>iova, iova);
+   err = -ENXIO;
+   goto err_unmap;
+   }
+
+   *addrp = addr;
+   *iovap = iova;
+   } else {
+   *addrp = sg_dma_address(sgt->sgl) + offset;
+   s

[PATCH 01/14] staging: media: tegra-vde: Support BSEV clock and reset

[Thierry Reding]

From: Thierry Reding 

The BSEV clock has a separate gate bit and can not be assumed to be
always enabled. Add explicit handling for the BSEV clock and reset.

This fixes an issue on Tegra124 where the BSEV clock is not enabled
by default and therefore accessing the BSEV registers will hang the
CPU if the BSEV clock is not enabled and the reset not deasserted.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 35 +++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 6f06061a40d9..9d8f833744db 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -74,9 +74,11 @@ struct tegra_vde {
struct miscdevice miscdev;
struct reset_control *rst;
struct reset_control *rst_mc;
+   struct reset_control *rst_bsev;
struct gen_pool *iram_pool;
struct completion decode_completion;
struct clk *clk;
+   struct clk *clk_bsev;
dma_addr_t iram_lists_addr;
u32 *iram;
 };
@@ -979,6 +981,11 @@ static int tegra_vde_runtime_suspend(struct device *dev)
return err;
}
 
+   reset_control_assert(vde->rst_bsev);
+
+   usleep_range(2000, 4000);
+
+   clk_disable_unprepare(vde->clk_bsev);
clk_disable_unprepare(vde->clk);
 
return 0;
@@ -996,6 +1003,16 @@ static int tegra_vde_runtime_resume(struct device *dev)
return err;
}
 
+   err = clk_prepare_enable(vde->clk_bsev);
+   if (err < 0)
+   return err;
+
+   err = reset_control_deassert(vde->rst_bsev);
+   if (err < 0)
+   return err;
+
+   usleep_range(2000, 4000);
+
return 0;
 }
 
@@ -1084,14 +1101,21 @@ static int tegra_vde_probe(struct platform_device *pdev)
if (IS_ERR(vde->frameid))
return PTR_ERR(vde->frameid);
 
-   vde->clk = devm_clk_get(dev, NULL);
+   vde->clk = devm_clk_get(dev, "vde");
if (IS_ERR(vde->clk)) {
err = PTR_ERR(vde->clk);
dev_err(dev, "Could not get VDE clk %d\n", err);
return err;
}
 
-   vde->rst = devm_reset_control_get(dev, NULL);
+   vde->clk_bsev = devm_clk_get(dev, "bsev");
+   if (IS_ERR(vde->clk_bsev)) {
+   err = PTR_ERR(vde->clk_bsev);
+   dev_err(dev, "failed to get BSEV clock: %d\n", err);
+   return err;
+   }
+
+   vde->rst = devm_reset_control_get(dev, "vde");
if (IS_ERR(vde->rst)) {
err = PTR_ERR(vde->rst);
dev_err(dev, "Could not get VDE reset %d\n", err);
@@ -1105,6 +1129,13 @@ static int tegra_vde_probe(struct platform_device *pdev)
return err;
}
 
+   vde->rst_bsev = devm_reset_control_get(dev, "bsev");
+   if (IS_ERR(vde->rst_bsev)) {
+   err = PTR_ERR(vde->rst_bsev);
+   dev_err(dev, "failed to get BSEV reset: %d\n", err);
+   return err;
+   }
+
irq = platform_get_irq_byname(pdev, "sync-token");
if (irq < 0)
return irq;
-- 
2.17.0

[PATCH 06/14] staging: media: tegra-vde: Print out invalid FD

[Thierry Reding]

From: Thierry Reding 

Include the invalid file descriptor when reporting an error message to
help diagnosing why importing the buffer failed.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 0ce30c7ccb75..0adc603fa437 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -643,7 +643,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf)) {
-   dev_err(dev, "Invalid dmabuf FD\n");
+   dev_err(dev, "Invalid dmabuf FD: %d\n", fd);
return PTR_ERR(dmabuf);
}
 
-- 
2.17.0

[PATCH 02/14] staging: media: tegra-vde: Support reference picture marking

[Thierry Reding]

From: Thierry Reding 

Tegra114 and Tegra124 support reference picture marking, which will
cause BSEV to write picture marking data to SDRAM. Make sure there is
a valid destination address for that data to avoid error messages from
the memory controller.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 54 -
 drivers/staging/media/tegra-vde/uapi.h  |  3 ++
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 9d8f833744db..3027b11b11ae 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -60,7 +60,12 @@ struct video_frame {
u32 flags;
 };
 
+struct tegra_vde_soc {
+   bool supports_ref_pic_marking;
+};
+
 struct tegra_vde {
+   const struct tegra_vde_soc *soc;
void __iomem *sxe;
void __iomem *bsev;
void __iomem *mbe;
@@ -330,6 +335,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
  struct video_frame *dpb_frames,
  dma_addr_t bitstream_data_addr,
  size_t bitstream_data_size,
+ dma_addr_t secure_addr,
  unsigned int macroblocks_nb)
 {
struct device *dev = vde->miscdev.parent;
@@ -454,6 +460,9 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
 
VDE_WR(bitstream_data_addr, vde->sxe + 0x6C);
 
+   if (vde->soc->supports_ref_pic_marking)
+   VDE_WR(secure_addr, vde->sxe + 0x7c);
+
value = 0x1005;
value |= ctx->pic_width_in_mbs << 11;
value |= ctx->pic_height_in_mbs << 3;
@@ -772,12 +781,15 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
struct tegra_vde_h264_frame __user *frames_user;
struct video_frame *dpb_frames;
struct dma_buf_attachment *bitstream_data_dmabuf_attachment;
-   struct sg_table *bitstream_sgt;
+   struct dma_buf_attachment *secure_attachment = NULL;
+   struct sg_table *bitstream_sgt, *secure_sgt;
enum dma_data_direction dma_dir;
dma_addr_t bitstream_data_addr;
+   dma_addr_t secure_addr;
dma_addr_t bsev_ptr;
size_t lsize, csize;
size_t bitstream_data_size;
+   size_t secure_size;
unsigned int macroblocks_nb;
unsigned int read_bytes;
unsigned int cstride;
@@ -803,6 +815,18 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
if (ret)
return ret;
 
+   if (vde->soc->supports_ref_pic_marking) {
+   ret = tegra_vde_attach_dmabuf(dev, ctx.secure_fd,
+ ctx.secure_offset, 0, SZ_256,
+ _attachment,
+ _addr,
+ _sgt,
+ _size,
+ DMA_TO_DEVICE);
+   if (ret)
+   goto release_bitstream_dmabuf;
+   }
+
dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
 GFP_KERNEL);
if (!dpb_frames) {
@@ -876,6 +900,7 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
ret = tegra_vde_setup_hw_context(vde, , dpb_frames,
 bitstream_data_addr,
 bitstream_data_size,
+secure_addr,
 macroblocks_nb);
if (ret)
goto put_runtime_pm;
@@ -929,6 +954,10 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
kfree(dpb_frames);
 
 release_bitstream_dmabuf:
+   if (secure_attachment)
+   tegra_vde_detach_and_put_dmabuf(secure_attachment, secure_sgt,
+   DMA_TO_DEVICE);
+
tegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment,
bitstream_sgt, DMA_TO_DEVICE);
 
@@ -1029,6 +1058,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, vde);
 
+   vde->soc = of_device_get_match_data(>dev);
+
regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sxe");
if (!regs)
return -ENODEV;
@@ -1258,8 +1289,27 @@ static const struct dev_pm_ops tegra_vde_pm_ops = {
tegra_vde_pm_resume)
 };
 
+static const struct tegra_vde_soc tegra20_vde_soc = {
+   .supports_ref_pic_marking = false,
+};
+
+static const struct tegra_vde_soc tegra30_vde_soc = {
+   .supports_ref_pic_marking = false,
+}

[PATCH 03/14] staging: media: tegra-vde: Prepare for interlacing support

[Thierry Reding]

From: Thierry Reding 

The number of frames doubles when decoding interlaced content and the
structures describing the frames double in size. Take that into account
to prepare for interlacing support.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 73 -
 1 file changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 3027b11b11ae..1a40f6dff7c8 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -61,7 +61,9 @@ struct video_frame {
 };
 
 struct tegra_vde_soc {
+   unsigned int num_ref_pics;
bool supports_ref_pic_marking;
+   bool supports_interlacing;
 };
 
 struct tegra_vde {
@@ -205,8 +207,12 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
u32 cr_addr = frame ? frame->cr_addr : 0x6CDEAD00;
u32 value1 = frame ? ((mbs_width << 16) | mbs_height) : 0;
u32 value2 = frame ? mbs_width + 1) >> 1) << 6) | 1) : 0;
+   u32 value = y_addr >> 8;
 
-   VDE_WR(y_addr  >> 8, vde->frameid + 0x000 + frameid * 4);
+   if (vde->soc->supports_interlacing)
+   value |= BIT(31);
+
+   VDE_WR(value,vde->frameid + 0x000 + frameid * 4);
VDE_WR(cb_addr >> 8, vde->frameid + 0x100 + frameid * 4);
VDE_WR(cr_addr >> 8, vde->frameid + 0x180 + frameid * 4);
VDE_WR(value1,   vde->frameid + 0x080 + frameid * 4);
@@ -229,20 +235,23 @@ static void tegra_setup_frameidx(struct tegra_vde *vde,
 }
 
 static void tegra_vde_setup_iram_entry(struct tegra_vde *vde,
+  unsigned int num_ref_pics,
   unsigned int table,
   unsigned int row,
   u32 value1, u32 value2)
 {
+   unsigned int entries = num_ref_pics * 2;
u32 *iram_tables = vde->iram;
 
dev_dbg(vde->miscdev.parent, "IRAM table %u: row %u: 0x%08X 0x%08X\n",
table, row, value1, value2);
 
-   iram_tables[0x20 * table + row * 2] = value1;
-   iram_tables[0x20 * table + row * 2 + 1] = value2;
+   iram_tables[entries * table + row * 2] = value1;
+   iram_tables[entries * table + row * 2 + 1] = value2;
 }
 
 static void tegra_vde_setup_iram_tables(struct tegra_vde *vde,
+   unsigned int num_ref_pics,
struct video_frame *dpb_frames,
unsigned int ref_frames_nb,
unsigned int with_earlier_poc_nb)
@@ -251,13 +260,17 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
u32 value, aux_addr;
int with_later_poc_nb;
unsigned int i, k;
+   size_t size;
+
+   size = num_ref_pics * 4 * 8;
+   memset(vde->iram, 0, size);
 
dev_dbg(vde->miscdev.parent, "DPB: Frame 0: frame_num = %d\n",
dpb_frames[0].frame_num);
 
dev_dbg(vde->miscdev.parent, "REF L0:\n");
 
-   for (i = 0; i < 16; i++) {
+   for (i = 0; i < num_ref_pics; i++) {
if (i < ref_frames_nb) {
frame = _frames[i + 1];
 
@@ -277,10 +290,14 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
value = 0;
}
 
-   tegra_vde_setup_iram_entry(vde, 0, i, value, aux_addr);
-   tegra_vde_setup_iram_entry(vde, 1, i, value, aux_addr);
-   tegra_vde_setup_iram_entry(vde, 2, i, value, aux_addr);
-   tegra_vde_setup_iram_entry(vde, 3, i, value, aux_addr);
+   tegra_vde_setup_iram_entry(vde, num_ref_pics, 0, i, value,
+  aux_addr);
+   tegra_vde_setup_iram_entry(vde, num_ref_pics, 1, i, value,
+  aux_addr);
+   tegra_vde_setup_iram_entry(vde, num_ref_pics, 2, i, value,
+  aux_addr);
+   tegra_vde_setup_iram_entry(vde, num_ref_pics, 3, i, value,
+  aux_addr);
}
 
if (!(dpb_frames[0].flags & FLAG_B_FRAME))
@@ -309,7 +326,8 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
"\tFrame %d: frame_num = %d\n",
k + 1, frame->frame_num);
 
-   tegra_vde_setup_iram_entry(vde, 2, i, value, aux_addr);
+   tegra_vde_setup_iram_entry(vde, num_ref_pics, 2, i, value,
+  aux_addr);
}
 
for (k = 0; i < ref_frames_nb; i++, k++) {
@@ -326,7 +344,8 @@ static void tegra_vde_setup

[PATCH 07/14] staging: media: tegra-vde: Add some clarifying comments

[Thierry Reding]

From: Thierry Reding 

Add some comments specifying what tables are being set up in VRAM.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 0adc603fa437..41cf86dc5dbd 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -271,6 +271,7 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
unsigned int i, k;
size_t size;
 
+   /* clear H256RefPicList */
size = num_ref_pics * 4 * 8;
memset(vde->iram, 0, size);
 
@@ -453,6 +454,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
VDE_WR(0x, vde->bsev + 0x98);
VDE_WR(0x0060, vde->bsev + 0x9C);
 
+   /* clear H264MB2SliceGroupMap, assuming no FMO */
memset(vde->iram + 1024, 0, macroblocks_nb / 2);
 
tegra_setup_frameidx(vde, dpb_frames, ctx->dpb_frames_nb,
@@ -480,6 +482,8 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
if (err)
return err;
 
+   /* upload H264MB2SliceGroupMap */
+   /* XXX don't hardcode map size? */
value = (0x20 << 26) | (0 << 25) | ((4096 >> 2) & 0x1fff);
err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false);
if (err)
@@ -492,6 +496,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
if (err)
return err;
 
+   /* clear H264MBInfo XXX don't hardcode size */
value = (0x21 << 26) | ((240 & 0x1fff) << 12) | (0x54c & 0xfff);
err = tegra_vde_push_to_bsev_icmdqueue(vde, 0x840F054C, false);
if (err)
@@ -499,6 +504,16 @@ static int tegra_vde_setup_hw_context(struct tegra_vde 
*vde,
 
size = num_ref_pics * 4 * 8;
 
+   /* clear H264RefPicList */
+   /*
+   value = (0x21 << 26) | (((size >> 2) & 0x1fff) << 12) | 0xE34;
+
+   err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false);
+   if (err)
+   return err;
+   */
+
+   /* upload H264RefPicList */
value = (0x20 << 26) | (0x0 << 25) | ((size >> 2) & 0x1fff);
err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false);
if (err)
@@ -584,7 +599,11 @@ static int tegra_vde_setup_hw_context(struct tegra_vde 
*vde,
 
tegra_vde_mbe_set_0xa_reg(vde, 0, 0x09FC);
tegra_vde_mbe_set_0xa_reg(vde, 2, 0x61DEAD00);
+#if 0
+   tegra_vde_mbe_set_0xa_reg(vde, 4, dpb_frames[0].aux_addr); /* 
0x62DEAD00 */
+#else
tegra_vde_mbe_set_0xa_reg(vde, 4, 0x62DEAD00);
+#endif
tegra_vde_mbe_set_0xa_reg(vde, 6, 0x63DEAD00);
tegra_vde_mbe_set_0xa_reg(vde, 8, dpb_frames[0].aux_addr);
 
-- 
2.17.0

[PATCH 05/14] staging: media: tegra-vde: Properly mark invalid entries

[Thierry Reding]

From: Thierry Reding 

Entries in the reference picture list are marked as invalid by setting
the frame ID to 0x3f.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 275884e745df..0ce30c7ccb75 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -296,7 +296,7 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
(frame->flags & FLAG_B_FRAME));
} else {
aux_addr = 0x6ADEAD00;
-   value = 0;
+   value = 0x3f;
}
 
tegra_vde_setup_iram_entry(vde, num_ref_pics, 0, i, value,
-- 
2.17.0

[PATCH 00/14] staging: media: tegra-vdea: Add Tegra124 support

[Thierry Reding]

From: Thierry Reding 

Hi,

this set of patches perform a bit of cleanup and extend support to the
VDE implementation found on Tegra114 and Tegra124. This requires adding
handling for a clock and a reset for the BSEV block that is separate
from the main VDE block. The new VDE revision also supports reference
picture marking, which requires that the BSEV writes out some related
data to a memory location. Since the supported tiling layouts have been
changed in Tegra124, which supports only block-linear and no pitch-
linear layouts, a new way is added to request a specific layout for the
decoded frames. Both of the above changes require breaking the ABI to
accomodate for the new data in the custom IOCTL.

Finally this set also adds support for dealing with an IOMMU, which
makes it more convenient to deal with imported buffers since they no
longer need to be physically contiguous.

Userspace changes for the updated ABI are available here:

https://cgit.freedesktop.org/~tagr/libvdpau-tegra/commit/

Mauro, I'm sending the device tree changes as part of the series for
completeness, but I expect to pick those up into the Tegra tree once
this has been reviewed and you've applied the driver changes.

Thanks,
Thierry

Thierry Reding (14):
  staging: media: tegra-vde: Support BSEV clock and reset
  staging: media: tegra-vde: Support reference picture marking
  staging: media: tegra-vde: Prepare for interlacing support
  staging: media: tegra-vde: Use DRM/KMS framebuffer modifiers
  staging: media: tegra-vde: Properly mark invalid entries
  staging: media: tegra-vde: Print out invalid FD
  staging: media: tegra-vde: Add some clarifying comments
  staging: media: tegra-vde: Track struct device *
  staging: media: tegra-vde: Add IOMMU support
  staging: media: tegra-vde: Keep VDE in reset when unused
  ARM: tegra: Enable VDE on Tegra124
  ARM: tegra: Add BSEV clock and reset for VDE on Tegra20
  ARM: tegra: Add BSEV clock and reset for VDE on Tegra30
  ARM: tegra: Enable SMMU for VDE on Tegra124

 arch/arm/boot/dts/tegra124.dtsi |  42 ++
 arch/arm/boot/dts/tegra20.dtsi  |  10 +-
 arch/arm/boot/dts/tegra30.dtsi  |  10 +-
 drivers/staging/media/tegra-vde/tegra-vde.c | 528 +---
 drivers/staging/media/tegra-vde/uapi.h  |   6 +-
 5 files changed, 511 insertions(+), 85 deletions(-)

-- 
2.17.0

[PATCH 04/14] staging: media: tegra-vde: Use DRM/KMS framebuffer modifiers

[Thierry Reding]

From: Thierry Reding 

VDE on Tegra20 through Tegra114 supports reading and writing frames in
16x16 tiled layout. Similarily, the various block-linear layouts that
are supported by the GPU on Tegra124 can also be read from and written
to by the Tegra124 VDE.

Enable userspace to specify the desired layout using the existing DRM
framebuffer modifiers.

Signed-off-by: Thierry Reding 
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 112 +---
 drivers/staging/media/tegra-vde/uapi.h  |   3 +-
 2 files changed, 100 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c 
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 1a40f6dff7c8..275884e745df 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -24,6 +24,8 @@
 
 #include 
 
+#include 
+
 #include "uapi.h"
 
 #define ICMDQUE_WR 0x00
@@ -58,12 +60,14 @@ struct video_frame {
dma_addr_t aux_addr;
u32 frame_num;
u32 flags;
+   u64 modifier;
 };
 
 struct tegra_vde_soc {
unsigned int num_ref_pics;
bool supports_ref_pic_marking;
bool supports_interlacing;
+   bool supports_block_linear;
 };
 
 struct tegra_vde {
@@ -202,6 +206,7 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
unsigned int frameid,
u32 mbs_width, u32 mbs_height)
 {
+   u64 modifier = frame ? frame->modifier : DRM_FORMAT_MOD_LINEAR;
u32 y_addr  = frame ? frame->y_addr  : 0x6CDEAD00;
u32 cb_addr = frame ? frame->cb_addr : 0x6CDEAD00;
u32 cr_addr = frame ? frame->cr_addr : 0x6CDEAD00;
@@ -209,8 +214,12 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
u32 value2 = frame ? mbs_width + 1) >> 1) << 6) | 1) : 0;
u32 value = y_addr >> 8;
 
-   if (vde->soc->supports_interlacing)
+   if (!vde->soc->supports_interlacing) {
+   if (modifier == DRM_FORMAT_MOD_NVIDIA_TEGRA_TILED)
+   value |= BIT(31);
+   } else {
value |= BIT(31);
+   }
 
VDE_WR(value,vde->frameid + 0x000 + frameid * 4);
VDE_WR(cb_addr >> 8, vde->frameid + 0x100 + frameid * 4);
@@ -349,6 +358,37 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde 
*vde,
}
 }
 
+static int tegra_vde_get_block_height(u64 modifier, unsigned int *block_height)
+{
+   switch (modifier) {
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_ONE_GOB:
+   *block_height = 0;
+   return 0;
+
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_TWO_GOB:
+   *block_height = 1;
+   return 0;
+
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_FOUR_GOB:
+   *block_height = 2;
+   return 0;
+
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_EIGHT_GOB:
+   *block_height = 3;
+   return 0;
+
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_SIXTEEN_GOB:
+   *block_height = 4;
+   return 0;
+
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_THIRTYTWO_GOB:
+   *block_height = 5;
+   return 0;
+   }
+
+   return -EINVAL;
+}
+
 static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
  struct tegra_vde_h264_decoder_ctx *ctx,
  struct video_frame *dpb_frames,
@@ -383,7 +423,21 @@ static int tegra_vde_setup_hw_context(struct tegra_vde 
*vde,
tegra_vde_set_bits(vde, 0x0005, vde->vdma + 0x04);
 
VDE_WR(0x, vde->vdma + 0x1C);
-   VDE_WR(0x, vde->vdma + 0x00);
+
+   value = 0x;
+
+   if (vde->soc->supports_block_linear) {
+   unsigned int block_height;
+
+   err = tegra_vde_get_block_height(dpb_frames[0].modifier,
+_height);
+   if (err < 0)
+   return err;
+
+   value |= block_height << 10;
+   }
+
+   VDE_WR(value, vde->vdma + 0x00);
VDE_WR(0x0007, vde->vdma + 0x04);
VDE_WR(0x0007, vde->frameid + 0x200);
VDE_WR(0x0005, vde->tfe + 0x04);
@@ -730,11 +784,37 @@ static void tegra_vde_release_frame_dmabufs(struct 
video_frame *frame,
 static int tegra_vde_validate_frame(struct device *dev,
struct tegra_vde_h264_frame *frame)
 {
+   struct tegra_vde *vde = dev_get_drvdata(dev);
+
if (frame->frame_num > 0x7F) {
dev_err(dev, "Bad frame_num %u\n", frame->frame_num);
return -EINVAL;
}
 
+   if (vde->soc->supports_block_linear) {
+   switch (frame->modifier) {
+   case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_ONE_GOB:

Re: [Linaro-mm-sig] [PATCH 4/8] dma-buf: add peer2peer flag

[Thierry Reding]

On Wed, Apr 25, 2018 at 09:30:39AM +0200, Daniel Vetter wrote:
> On Wed, Apr 25, 2018 at 12:09:05AM -0700, Christoph Hellwig wrote:
> > On Wed, Apr 25, 2018 at 09:02:17AM +0200, Daniel Vetter wrote:
> > > Can we please not nack everything right away? Doesn't really motivate
> > > me to show you all the various things we're doing in gpu to make the
> > > dma layer work for us. That kind of noodling around in lower levels to
> > > get them to do what we want is absolutely par-for-course for gpu
> > > drivers. If you just nack everything I point you at for illustrative
> > > purposes, then I can't show you stuff anymore.
> > 
> > No, it's not.  No driver (and that includes the magic GPUs) has
> > any business messing with dma ops directly.
> > 
> > A GPU driver imght have a very valid reason to disable the IOMMU,
> > but the code to do so needs to be at least in the arch code, maybe
> > in the dma-mapping/iommu code, not in the driver.
> > 
> > As a first step to get the discussion started we'll simply need
> > to move the code Thierry wrote into a helper in arch/arm and that
> > alone would be a massive improvement.  I'm not even talking about
> > minor details like actually using arm_get_dma_map_ops instead
> > of duplicating it.
> > 
> > And doing this basic trivial work really helps to get this whole
> > mess under control.
> 
> Ah ok. It did sound a bit like a much more cathegorical NAK than an "ack
> in principle, but we need to shuffle the implementation into the right
> place first". In the past we generally got a principled NAK on anything
> funny we've been doing with the dma api, and the dma api maintainer
> steaming off telling us we're incompetent idiots. I guess I've been
> branded a bit on this topic :-/
> 
> Really great that this is changing now.
> 
> On the patch itself: It might not be the right thing in all cases, since
> for certain compression formats the nv gpu wants larger pages (easy to
> allocate from vram, not so easy from main memory), so might need the iommu
> still. But currently that's not implemented:
> 
> https://www.spinics.net/lists/dri-devel/msg173932.html

To clarify: we do want to use the IOMMU, but we want to use it
explicitly via the IOMMU API rather than hiding it behind the DMA API.
We do the same thing in Tegra DRM where we don't want to use the DMA API
because it doesn't allow us to share the same mapping between multiple
display controllers in the same way the IOMMU API does. We've also been
thinking about using the IOMMU API directly in order to support process
isolation for devices that accept command streams from userspace.

Fortunately the issue I'm seeing with Nouveau doesn't happen with Tegra
DRM, which seems to be because we have an IOMMU group with multiple
devices and that prevents the DMA API from "hijacking" the IOMMU domain
for the group.

And to add to the confusion, none of this seems to be an issue on 64-bit
ARM where the generic DMA/IOMMU code from drivers/iommu/dma-iommu.c is
used.

Thierry


signature.asc
Description: PGP signature

Re: [Linaro-mm-sig] [PATCH 4/8] dma-buf: add peer2peer flag

[Thierry Reding]

On Wed, Apr 25, 2018 at 12:09:05AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 25, 2018 at 09:02:17AM +0200, Daniel Vetter wrote:
> > Can we please not nack everything right away? Doesn't really motivate
> > me to show you all the various things we're doing in gpu to make the
> > dma layer work for us. That kind of noodling around in lower levels to
> > get them to do what we want is absolutely par-for-course for gpu
> > drivers. If you just nack everything I point you at for illustrative
> > purposes, then I can't show you stuff anymore.
> 
> No, it's not.  No driver (and that includes the magic GPUs) has
> any business messing with dma ops directly.
> 
> A GPU driver imght have a very valid reason to disable the IOMMU,
> but the code to do so needs to be at least in the arch code, maybe
> in the dma-mapping/iommu code, not in the driver.
> 
> As a first step to get the discussion started we'll simply need
> to move the code Thierry wrote into a helper in arch/arm and that
> alone would be a massive improvement.  I'm not even talking about
> minor details like actually using arm_get_dma_map_ops instead
> of duplicating it.
> 
> And doing this basic trivial work really helps to get this whole
> mess under control.

That's a good idea and I can prepare patches for this, but I'd like to
make those changes on top of the fix to keep the option of getting this
into v4.16 if at all possible.

Thierry


signature.asc
Description: PGP signature

Re: [Linaro-mm-sig] [PATCH 4/8] dma-buf: add peer2peer flag

[Thierry Reding]

On Tue, Apr 24, 2018 at 11:43:35PM -0700, Christoph Hellwig wrote:
> On Wed, Apr 25, 2018 at 08:23:15AM +0200, Daniel Vetter wrote:
> > For more fun:
> > 
> > https://www.spinics.net/lists/dri-devel/msg173630.html
> > 
> > Yeah, sometimes we want to disable the iommu because the on-gpu
> > pagetables are faster ...
> 
> I am not on this list, but remote NAK from here.  This needs an
> API from the iommu/dma-mapping code.  Drivers have no business poking
> into these details.

The interfaces that the above patch uses are all EXPORT_SYMBOL_GPL,
which is rather misleading if they are not meant to be used by drivers
directly.

> Thierry, please resend this with at least the iommu list and
> linux-arm-kernel in Cc to have a proper discussion on the right API.

I'm certainly open to help with finding a correct solution, but the
patch above was purposefully terse because this is something that I
hope we can get backported to v4.16 to unbreak Nouveau. Coordinating
such a backport between ARM and DRM trees does not sound like something
that would help getting this fixed in v4.16.

The fundamental issue here is that the DMA/IOMMU integration is
something that has caused a number of surprising regressions in the past
because it tends to sneak in unexpectedly. For example the current
regression shows up only if CONFIG_ARM_DMA_USE_IOMMU=y because the DMA
API will then transparently create a second mapping and mess things up.
Everything works fine if that option is disabled. This is ultimately why
we didn't notice, since we don't enable that option by default. I do
have a patch that I plan to apply to the Tegra tree that will always
enable CONFIG_ARM_DMA_USE_IOMMU=y on Tegra to avoid any such surprises
in the future, but I can obviously only apply that once the above patch
is applied to Nouveau, otherwise we'll break Nouveau unconditionally.

Granted, this issue could've been caught with a little more testing, but
in retrospect I think it would've been a lot better if ARM_DMA_USE_IOMMU
was just enabled unconditionally if it has side-effects that platforms
don't opt in to but have to explicitly opt out of.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Thierry Reding]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)

I don't think it makes sense to leak mode support into the legacy API.
The pwm_config() function is considered legacy and should eventually go
away. As such it doesn't make sense to integrate a new feature such as
PWM modes into it. All users of pwm_config() assume normal mode, and
that's what pwm_config() should provide.

Anyone that needs something other than normal mode should use the new
atomic PWM API.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
>  wrote:
> > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> >>  wrote:
> >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> >> >  wrote:
> >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> 
> >>
> >> At one point we had discussed adding a 'dma-masters' property that
> >> lists all the buses on which a device can be a dma master, and
> >> the respective properties of those masters (iommu, coherency,
> >> offset, ...).
> >>
> >> IIRC at the time we decided that we could live without that complexity,
> >> but perhaps we cannot.
> >
> > Are you talking about this ?
> > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> >
> > It doesn't seem to be related to that issue to me. And in our
> > particular cases, all the devices are DMA masters, the RAM is just
> > mapped to another address.
> 
> No, that's not the one I was thinking of. The idea at the time was much
> more generic, and not limited to dma engines. I don't recall the details,
> but I think that Thierry was either involved or made the proposal at the
> time.

Yeah, I vaguely remember discussing something like this before. A quick
search through my inbox yielded these two threads, mostly related to
IOMMU but I think there were some mentions about dma-ranges and so on as
well. I'll have to dig deeper into those threads to refresh my memories,
but I won't get around to it until later today.

If someone wants to read up on this in the meantime, here are the links:

https://lkml.org/lkml/2014/4/27/346

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html

From a quick glance the issue of dma-ranges was something that we hand-
waved at the time.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
> On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
> > <maxime.rip...@free-electrons.com> wrote:
> > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> > >> <linus.wall...@linaro.org> wrote:
> > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> > >> > <maxime.rip...@free-electrons.com> wrote:
> > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> > 
> > >>
> > >> At one point we had discussed adding a 'dma-masters' property that
> > >> lists all the buses on which a device can be a dma master, and
> > >> the respective properties of those masters (iommu, coherency,
> > >> offset, ...).
> > >>
> > >> IIRC at the time we decided that we could live without that complexity,
> > >> but perhaps we cannot.
> > >
> > > Are you talking about this ?
> > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> > >
> > > It doesn't seem to be related to that issue to me. And in our
> > > particular cases, all the devices are DMA masters, the RAM is just
> > > mapped to another address.
> > 
> > No, that's not the one I was thinking of. The idea at the time was much
> > more generic, and not limited to dma engines. I don't recall the details,
> > but I think that Thierry was either involved or made the proposal at the
> > time.
> 
> Yeah, I vaguely remember discussing something like this before. A quick
> search through my inbox yielded these two threads, mostly related to
> IOMMU but I think there were some mentions about dma-ranges and so on as
> well. I'll have to dig deeper into those threads to refresh my memories,
> but I won't get around to it until later today.
> 
> If someone wants to read up on this in the meantime, here are the links:
> 
>   https://lkml.org/lkml/2014/4/27/346
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
> 
> From a quick glance the issue of dma-ranges was something that we hand-
> waved at the time.
> 
> Thierry

Also found this, which seems to be relevant as well:


http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html

Adding Dave.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v5 4/4] ARM: dts: tegra20: Add video decoder node

[Thierry Reding]

On Tue, Dec 12, 2017 at 03:26:10AM +0300, Dmitry Osipenko wrote:
> Add Video Decoder Engine device node.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 27 +++
>  1 file changed, 27 insertions(+)

Applied, thanks.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v5 3/4] ARM: dts: tegra20: Add device tree node to describe IRAM

[Thierry Reding]

On Tue, Dec 12, 2017 at 03:26:09AM +0300, Dmitry Osipenko wrote:
> From: Vladimir Zapolskiy 
> 
> All Tegra20 SoCs contain 256KiB IRAM, which is used to store
> resume code and by a video decoder engine.
> 
> Signed-off-by: Vladimir Zapolskiy 
> Signed-off-by: Dmitry Osipenko 
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 8 
>  1 file changed, 8 insertions(+)

Applied, thanks.

Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 4/4] drm/tegra: add cec-notifier support

[Thierry Reding]

On Thu, Oct 19, 2017 at 03:37:43PM +0200, Hans Verkuil wrote:
> On 10/19/17 15:30, Thierry Reding wrote:
> > On Thu, Oct 19, 2017 at 03:17:16PM +0200, Thierry Reding wrote:
> >> On Mon, Sep 11, 2017 at 02:29:52PM +0200, Hans Verkuil wrote:
> >>> From: Hans Verkuil <hans.verk...@cisco.com>
> >>>
> >>> In order to support CEC the HDMI driver has to inform the CEC driver
> >>> whenever the physical address changes. So when the EDID is read the
> >>> CEC driver has to be informed and whenever the hotplug detect goes
> >>> away.
> >>>
> >>> This is done through the cec-notifier framework.
> >>>
> >>> The link between the HDMI driver and the CEC driver is done through
> >>> the hdmi_phandle in the tegra-cec node in the device tree.
> >>>
> >>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> >>> ---
> >>>  drivers/gpu/drm/tegra/Kconfig  | 1 +
> >>>  drivers/gpu/drm/tegra/drm.h| 3 +++
> >>>  drivers/gpu/drm/tegra/hdmi.c   | 9 +
> >>>  drivers/gpu/drm/tegra/output.c | 6 ++
> >>>  4 files changed, 19 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> >>> index 2db29d67193d..c882918c2024 100644
> >>> --- a/drivers/gpu/drm/tegra/Kconfig
> >>> +++ b/drivers/gpu/drm/tegra/Kconfig
> >>> @@ -8,6 +8,7 @@ config DRM_TEGRA
> >>>   select DRM_PANEL
> >>>   select TEGRA_HOST1X
> >>>   select IOMMU_IOVA if IOMMU_SUPPORT
> >>> + select CEC_CORE if CEC_NOTIFIER
> >>>   help
> >>> Choose this option if you have an NVIDIA Tegra SoC.
> >>>  
> >>> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> >>> index 6d6da01282f3..c0a18b60caf1 100644
> >>> --- a/drivers/gpu/drm/tegra/drm.h
> >>> +++ b/drivers/gpu/drm/tegra/drm.h
> >>> @@ -212,6 +212,8 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> >>>  struct clk *clk, unsigned long pclk,
> >>>  unsigned int div);
> >>>  
> >>> +struct cec_notifier;
> >>> +
> >>>  struct tegra_output {
> >>>   struct device_node *of_node;
> >>>   struct device *dev;
> >>> @@ -219,6 +221,7 @@ struct tegra_output {
> >>>   struct drm_panel *panel;
> >>>   struct i2c_adapter *ddc;
> >>>   const struct edid *edid;
> >>> + struct cec_notifier *notifier;
> >>>   unsigned int hpd_irq;
> >>>   int hpd_gpio;
> >>>   enum of_gpio_flags hpd_gpio_flags;
> >>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> >>> index cda0491ed6bf..fbf14e1efd0e 100644
> >>> --- a/drivers/gpu/drm/tegra/hdmi.c
> >>> +++ b/drivers/gpu/drm/tegra/hdmi.c
> >>> @@ -21,6 +21,8 @@
> >>>  
> >>>  #include 
> >>>  
> >>> +#include 
> >>> +
> >>>  #include "hdmi.h"
> >>>  #include "drm.h"
> >>>  #include "dc.h"
> >>> @@ -1720,6 +1722,10 @@ static int tegra_hdmi_probe(struct platform_device 
> >>> *pdev)
> >>>   return PTR_ERR(hdmi->vdd);
> >>>   }
> >>>  
> >>> + hdmi->output.notifier = cec_notifier_get(>dev);
> >>> + if (hdmi->output.notifier == NULL)
> >>> + return -ENOMEM;
> >>> +
> >>>   hdmi->output.dev = >dev;
> >>>  
> >>>   err = tegra_output_probe(>output);
> >>> @@ -1778,6 +1784,9 @@ static int tegra_hdmi_remove(struct platform_device 
> >>> *pdev)
> >>>  
> >>>   tegra_output_remove(>output);
> >>>  
> >>> + if (hdmi->output.notifier)
> >>> + cec_notifier_put(hdmi->output.notifier);
> >>> +
> >>>   return 0;
> >>>  }
> >>>  
> >>> diff --git a/drivers/gpu/drm/tegra/output.c 
> >>> b/drivers/gpu/drm/tegra/output.c
> >>> index 595d1ec3e02e..57c052521a44 100644
> >>> --- a/drivers/gpu/drm/tegra/output.c
> >>> +++ b/drivers/gpu/drm/tegra/output.c
> >>> @@ -11,6 +11,8 @@
> >>>  #include 
> >>>  #include "drm.h"
> >>>  
> >>> +#include 
> >>> +
> >>

Re: [PATCHv4 4/4] drm/tegra: add cec-notifier support

[Thierry Reding]

On Thu, Oct 19, 2017 at 03:17:16PM +0200, Thierry Reding wrote:
> On Mon, Sep 11, 2017 at 02:29:52PM +0200, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verk...@cisco.com>
> > 
> > In order to support CEC the HDMI driver has to inform the CEC driver
> > whenever the physical address changes. So when the EDID is read the
> > CEC driver has to be informed and whenever the hotplug detect goes
> > away.
> > 
> > This is done through the cec-notifier framework.
> > 
> > The link between the HDMI driver and the CEC driver is done through
> > the hdmi_phandle in the tegra-cec node in the device tree.
> > 
> > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> > ---
> >  drivers/gpu/drm/tegra/Kconfig  | 1 +
> >  drivers/gpu/drm/tegra/drm.h| 3 +++
> >  drivers/gpu/drm/tegra/hdmi.c   | 9 +
> >  drivers/gpu/drm/tegra/output.c | 6 ++
> >  4 files changed, 19 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> > index 2db29d67193d..c882918c2024 100644
> > --- a/drivers/gpu/drm/tegra/Kconfig
> > +++ b/drivers/gpu/drm/tegra/Kconfig
> > @@ -8,6 +8,7 @@ config DRM_TEGRA
> > select DRM_PANEL
> > select TEGRA_HOST1X
> > select IOMMU_IOVA if IOMMU_SUPPORT
> > +   select CEC_CORE if CEC_NOTIFIER
> > help
> >   Choose this option if you have an NVIDIA Tegra SoC.
> >  
> > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> > index 6d6da01282f3..c0a18b60caf1 100644
> > --- a/drivers/gpu/drm/tegra/drm.h
> > +++ b/drivers/gpu/drm/tegra/drm.h
> > @@ -212,6 +212,8 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> >struct clk *clk, unsigned long pclk,
> >unsigned int div);
> >  
> > +struct cec_notifier;
> > +
> >  struct tegra_output {
> > struct device_node *of_node;
> > struct device *dev;
> > @@ -219,6 +221,7 @@ struct tegra_output {
> > struct drm_panel *panel;
> > struct i2c_adapter *ddc;
> > const struct edid *edid;
> > +   struct cec_notifier *notifier;
> > unsigned int hpd_irq;
> > int hpd_gpio;
> > enum of_gpio_flags hpd_gpio_flags;
> > diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> > index cda0491ed6bf..fbf14e1efd0e 100644
> > --- a/drivers/gpu/drm/tegra/hdmi.c
> > +++ b/drivers/gpu/drm/tegra/hdmi.c
> > @@ -21,6 +21,8 @@
> >  
> >  #include 
> >  
> > +#include 
> > +
> >  #include "hdmi.h"
> >  #include "drm.h"
> >  #include "dc.h"
> > @@ -1720,6 +1722,10 @@ static int tegra_hdmi_probe(struct platform_device 
> > *pdev)
> > return PTR_ERR(hdmi->vdd);
> > }
> >  
> > +   hdmi->output.notifier = cec_notifier_get(>dev);
> > +   if (hdmi->output.notifier == NULL)
> > +   return -ENOMEM;
> > +
> > hdmi->output.dev = >dev;
> >  
> > err = tegra_output_probe(>output);
> > @@ -1778,6 +1784,9 @@ static int tegra_hdmi_remove(struct platform_device 
> > *pdev)
> >  
> > tegra_output_remove(>output);
> >  
> > +   if (hdmi->output.notifier)
> > +   cec_notifier_put(hdmi->output.notifier);
> > +
> > return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> > index 595d1ec3e02e..57c052521a44 100644
> > --- a/drivers/gpu/drm/tegra/output.c
> > +++ b/drivers/gpu/drm/tegra/output.c
> > @@ -11,6 +11,8 @@
> >  #include 
> >  #include "drm.h"
> >  
> > +#include 
> > +
> >  int tegra_output_connector_get_modes(struct drm_connector *connector)
> >  {
> > struct tegra_output *output = connector_to_output(connector);
> > @@ -33,6 +35,7 @@ int tegra_output_connector_get_modes(struct drm_connector 
> > *connector)
> > edid = drm_get_edid(connector, output->ddc);
> >  
> > drm_mode_connector_update_edid_property(connector, edid);
> > +   cec_notifier_set_phys_addr_from_edid(output->notifier, edid);
> 
> I had to guard this with:
> 
>   #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>   ...
>   #endif
> 
> to enable this driver to be built without CEC_NOTIFIER enabled. I see
> that there are stubs defined if the above is false, but for some reason
> they don't seem to be there for me either.

Nevermind that

Re: [PATCHv4 4/4] drm/tegra: add cec-notifier support

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:52PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> In order to support CEC the HDMI driver has to inform the CEC driver
> whenever the physical address changes. So when the EDID is read the
> CEC driver has to be informed and whenever the hotplug detect goes
> away.
> 
> This is done through the cec-notifier framework.
> 
> The link between the HDMI driver and the CEC driver is done through
> the hdmi_phandle in the tegra-cec node in the device tree.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/gpu/drm/tegra/Kconfig  | 1 +
>  drivers/gpu/drm/tegra/drm.h| 3 +++
>  drivers/gpu/drm/tegra/hdmi.c   | 9 +
>  drivers/gpu/drm/tegra/output.c | 6 ++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 2db29d67193d..c882918c2024 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -8,6 +8,7 @@ config DRM_TEGRA
>   select DRM_PANEL
>   select TEGRA_HOST1X
>   select IOMMU_IOVA if IOMMU_SUPPORT
> + select CEC_CORE if CEC_NOTIFIER
>   help
> Choose this option if you have an NVIDIA Tegra SoC.
>  
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 6d6da01282f3..c0a18b60caf1 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -212,6 +212,8 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>  struct clk *clk, unsigned long pclk,
>  unsigned int div);
>  
> +struct cec_notifier;
> +
>  struct tegra_output {
>   struct device_node *of_node;
>   struct device *dev;
> @@ -219,6 +221,7 @@ struct tegra_output {
>   struct drm_panel *panel;
>   struct i2c_adapter *ddc;
>   const struct edid *edid;
> + struct cec_notifier *notifier;
>   unsigned int hpd_irq;
>   int hpd_gpio;
>   enum of_gpio_flags hpd_gpio_flags;
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index cda0491ed6bf..fbf14e1efd0e 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -21,6 +21,8 @@
>  
>  #include 
>  
> +#include 
> +
>  #include "hdmi.h"
>  #include "drm.h"
>  #include "dc.h"
> @@ -1720,6 +1722,10 @@ static int tegra_hdmi_probe(struct platform_device 
> *pdev)
>   return PTR_ERR(hdmi->vdd);
>   }
>  
> + hdmi->output.notifier = cec_notifier_get(>dev);
> + if (hdmi->output.notifier == NULL)
> + return -ENOMEM;
> +
>   hdmi->output.dev = >dev;
>  
>   err = tegra_output_probe(>output);
> @@ -1778,6 +1784,9 @@ static int tegra_hdmi_remove(struct platform_device 
> *pdev)
>  
>   tegra_output_remove(>output);
>  
> + if (hdmi->output.notifier)
> + cec_notifier_put(hdmi->output.notifier);
> +
>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 595d1ec3e02e..57c052521a44 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -11,6 +11,8 @@
>  #include 
>  #include "drm.h"
>  
> +#include 
> +
>  int tegra_output_connector_get_modes(struct drm_connector *connector)
>  {
>   struct tegra_output *output = connector_to_output(connector);
> @@ -33,6 +35,7 @@ int tegra_output_connector_get_modes(struct drm_connector 
> *connector)
>   edid = drm_get_edid(connector, output->ddc);
>  
>   drm_mode_connector_update_edid_property(connector, edid);
> + cec_notifier_set_phys_addr_from_edid(output->notifier, edid);

I had to guard this with:

#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
...
#endif

to enable this driver to be built without CEC_NOTIFIER enabled. I see
that there are stubs defined if the above is false, but for some reason
they don't seem to be there for me either.

>  
>   if (edid) {
>   err = drm_add_edid_modes(connector, edid);
> @@ -68,6 +71,9 @@ tegra_output_connector_detect(struct drm_connector 
> *connector, bool force)
>   status = connector_status_connected;
>   }
>  
> + if (status != connector_status_connected)
> + cec_notifier_phys_addr_invalidate(output->notifier);

Same here.

Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 0/4] tegra-cec: add Tegra HDMI CEC support

[Thierry Reding]

On Thu, Oct 19, 2017 at 11:36:14AM +0200, Hans Verkuil wrote:
> On 10/19/17 11:20, Thierry Reding wrote:
> > On Mon, Sep 11, 2017 at 02:29:48PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verk...@cisco.com>
> >>
> >> This patch series adds support for the Tegra CEC functionality.
> >>
> >> This v4 has been rebased to the latest 4.14 pre-rc1 mainline.
> >>
> >> Please review! Other than for the bindings that are now Acked I have not
> >> received any feedback.
> >>
> >> The first patch documents the CEC bindings, the second adds support
> >> for this to tegra124.dtsi and enables it for the Jetson TK1.
> >>
> >> The third patch adds the CEC driver itself and the final patch adds
> >> the cec notifier support to the drm/tegra driver in order to notify
> >> the CEC driver whenever the physical address changes.
> >>
> >> I expect that the dts changes apply as well to the Tegra X1/X2 and possibly
> >> other Tegra SoCs, but I can only test this with my Jetson TK1 board.
> >>
> >> The dt-bindings and the tegra-cec driver would go in through the media
> >> subsystem, the drm/tegra part through the drm subsystem and the dts
> >> changes through (I guess) the linux-tegra developers. Luckily they are
> >> all independent of one another.
> >>
> >> To test this you need the CEC utilities from 
> >> git://linuxtv.org/v4l-utils.git.
> >>
> >> To build this:
> >>
> >> git clone git://linuxtv.org/v4l-utils.git
> >> cd v4l-utils
> >> ./bootstrap.sh; ./configure
> >> make
> >> sudo make install # optional, you really only need utils/cec*
> >>
> >> To test:
> >>
> >> cec-ctl --playback # configure as playback device
> >> cec-ctl -S # detect all connected CEC devices
> > 
> > I finally got around to test this. Unfortunately I wasn't able to
> > properly show connected CEC devices, but I think that may just be
> > because the monitor I was testing against doesn't support CEC. I
> > will have to check against a different device eventually to check
> > that it properly enumerates, though I suspect you've already done
> > quite extensive testing yourself.
> 
> Yes, you need a TV with CEC. Most (all?) PC monitors do not support this.
> I never understood why not.
> 
> So just to confirm: you've merged this driver for 4.15?

I've applied the drm/tegra bits to drm/tegra/for-next and pulled the ARM
DT changes into the Tegra tree for v4.15. I think we had agreed that you
would take the DT bindings and CEC driver through your tree?

Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 4/4] drm/tegra: add cec-notifier support

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:52PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> In order to support CEC the HDMI driver has to inform the CEC driver
> whenever the physical address changes. So when the EDID is read the
> CEC driver has to be informed and whenever the hotplug detect goes
> away.
> 
> This is done through the cec-notifier framework.
> 
> The link between the HDMI driver and the CEC driver is done through
> the hdmi_phandle in the tegra-cec node in the device tree.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/gpu/drm/tegra/Kconfig  | 1 +
>  drivers/gpu/drm/tegra/drm.h| 3 +++
>  drivers/gpu/drm/tegra/hdmi.c   | 9 +
>  drivers/gpu/drm/tegra/output.c | 6 ++
>  4 files changed, 19 insertions(+)

I've applied this to drm/tegra/for-next.

Thanks,
Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 3/4] tegra-cec: add Tegra HDMI CEC driver

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:51PM +0200, Hans Verkuil wrote:
[...]
> diff --git a/drivers/media/platform/tegra-cec/tegra_cec.c 
> b/drivers/media/platform/tegra-cec/tegra_cec.c
[...]
> +static int tegra_cec_probe(struct platform_device *pdev)
> +{
> + struct platform_device *hdmi_dev;
> + struct device_node *np;
> + struct tegra_cec *cec;
> + struct resource *res;
> + int ret = 0;
> +
> + np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
> +
> + if (!np) {
> + dev_err(>dev, "Failed to find hdmi node in device 
> tree\n");
> + return -ENODEV;
> + }
> + hdmi_dev = of_find_device_by_node(np);
> + if (hdmi_dev == NULL)
> + return -EPROBE_DEFER;

This seems a little awkward. Why exactly do we need to defer probe here?
It seems to me like cec_notifier_get() should be able to deal with HDMI
appearing at a later point.

> +
> + cec = devm_kzalloc(>dev, sizeof(struct tegra_cec), GFP_KERNEL);
> +
> + if (!cec)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + if (!res) {
> + dev_err(>dev,
> + "Unable to allocate resources for device\n");
> + ret = -EBUSY;
> + goto cec_error;
> + }
> +
> + if (!devm_request_mem_region(>dev, res->start, resource_size(res),
> + pdev->name)) {
> + dev_err(>dev,
> + "Unable to request mem region for device\n");
> + ret = -EBUSY;
> + goto cec_error;
> + }
> +
> + cec->tegra_cec_irq = platform_get_irq(pdev, 0);
> +
> + if (cec->tegra_cec_irq <= 0) {
> + ret = -EBUSY;
> + goto cec_error;
> + }
> +
> + cec->cec_base = devm_ioremap_nocache(>dev, res->start,
> + resource_size(res));
> +
> + if (!cec->cec_base) {
> + dev_err(>dev, "Unable to grab IOs for device\n");
> + ret = -EBUSY;
> + goto cec_error;
> + }
> +
> + cec->clk = devm_clk_get(>dev, "cec");
> +
> + if (IS_ERR_OR_NULL(cec->clk)) {
> + dev_err(>dev, "Can't get clock for CEC\n");
> + ret = -ENOENT;
> + goto clk_error;
> + }
> +
> + clk_prepare_enable(cec->clk);
> +
> + /* set context info. */
> + cec->dev = >dev;
> +
> + platform_set_drvdata(pdev, cec);
> +
> + ret = devm_request_threaded_irq(>dev, cec->tegra_cec_irq,
> + tegra_cec_irq_handler, tegra_cec_irq_thread_handler,
> + 0, "cec_irq", >dev);
> +
> + if (ret) {
> + dev_err(>dev,
> + "Unable to request interrupt for device\n");
> + goto cec_error;
> + }
> +
> + cec->notifier = cec_notifier_get(_dev->dev);
> + if (!cec->notifier) {
> + ret = -ENOMEM;
> + goto cec_error;
> + }

Ah... I see why we need the HDMI device right away. This seems a little
brittle to me, for two reasons: what if the HDMI controller goes away?
Will we be hanging on to a stale device? I mean, the device doesn't
necessarily have to go away, but what's the effect on CEC if the driver
unbinds from the HDMI controller?

Secondly, this creates a circular dependency. It seems to me like it'd
actually be simpler if the CEC controller was a "service provider" that
HDMI could use and "request/release" as appropriate.

In that case, the DT would look somewhat like this:

hdmi@... {
cec = <>;
};

cec: cec@... {
...
};

And then the HDMI driver could do something like:

cec = cec_get(>dev);
/* register notifier, ... */

That way the dependency becomes unidirectional and it seems to me like
that would allow interactions between HDMI and CEC would become simpler
overall.

Anyway, this is something that could always be changed after the fact
(except maybe for some bits needed for backwards-compatibility with old
device trees), and this seems to work well enough as it is, so:

Acked-by: Thierry Reding <tred...@nvidia.com>


signature.asc
Description: PGP signature

Re: [PATCHv4 2/4] ARM: tegra: add CEC support to tegra124.dtsi

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:50PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add support for the Tegra CEC IP to tegra124.dtsi and enable it on the
> Jetson TK1.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  arch/arm/boot/dts/tegra124-jetson-tk1.dts |  4 
>  arch/arm/boot/dts/tegra124.dtsi   | 12 +++-
>  2 files changed, 15 insertions(+), 1 deletion(-)

I prefer SoC and board changes to be split into separate patches. I've
done that with this patch while applying.

Thanks,
Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 1/4] dt-bindings: document the tegra CEC bindings

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:49PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
> 
> This documents the binding for the Tegra CEC module.
> 
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Rob Herring <r...@kernel.org>
> ---
>  .../devicetree/bindings/media/tegra-cec.txt| 27 
> ++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/tegra-cec.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/tegra-cec.txt 
> b/Documentation/devicetree/bindings/media/tegra-cec.txt
> new file mode 100644
> index ..c503f06f3b84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/tegra-cec.txt
> @@ -0,0 +1,27 @@
> +* Tegra HDMI CEC hardware
> +
> +The HDMI CEC module is present in Tegra SoCs and its purpose is to
> +handle communication between HDMI connected devices over the CEC bus.
> +
> +Required properties:
> +  - compatible : value should be one of the following:
> + "nvidia,tegra114-cec"
> + "nvidia,tegra124-cec"
> + "nvidia,tegra210-cec"
> +  - reg : Physical base address of the IP registers and length of memory
> +   mapped region.
> +  - interrupts : HDMI CEC interrupt number to the CPU.
> +  - clocks : from common clock binding: handle to HDMI CEC clock.
> +  - clock-names : from common clock binding: must contain "cec",
> +   corresponding to the entry in the clocks property.
> +  - hdmi-phandle : phandle to the HDMI controller, see also cec.txt.

I don't understand the need for the -phandle suffix. I would've probably
just gone with "hdmi", or "hdmi-controller". But I see that this is
already pretty standardized, so...

Acked-by: Thierry Reding <tred...@nvidia.com>


signature.asc
Description: PGP signature

Re: [PATCHv4 0/4] tegra-cec: add Tegra HDMI CEC support

[Thierry Reding]

On Mon, Sep 11, 2017 at 02:29:48PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This patch series adds support for the Tegra CEC functionality.
> 
> This v4 has been rebased to the latest 4.14 pre-rc1 mainline.
> 
> Please review! Other than for the bindings that are now Acked I have not
> received any feedback.
> 
> The first patch documents the CEC bindings, the second adds support
> for this to tegra124.dtsi and enables it for the Jetson TK1.
> 
> The third patch adds the CEC driver itself and the final patch adds
> the cec notifier support to the drm/tegra driver in order to notify
> the CEC driver whenever the physical address changes.
> 
> I expect that the dts changes apply as well to the Tegra X1/X2 and possibly
> other Tegra SoCs, but I can only test this with my Jetson TK1 board.
> 
> The dt-bindings and the tegra-cec driver would go in through the media
> subsystem, the drm/tegra part through the drm subsystem and the dts
> changes through (I guess) the linux-tegra developers. Luckily they are
> all independent of one another.
> 
> To test this you need the CEC utilities from git://linuxtv.org/v4l-utils.git.
> 
> To build this:
> 
> git clone git://linuxtv.org/v4l-utils.git
> cd v4l-utils
> ./bootstrap.sh; ./configure
> make
> sudo make install # optional, you really only need utils/cec*
> 
> To test:
> 
> cec-ctl --playback # configure as playback device
> cec-ctl -S # detect all connected CEC devices

I finally got around to test this. Unfortunately I wasn't able to
properly show connected CEC devices, but I think that may just be
because the monitor I was testing against doesn't support CEC. I
will have to check against a different device eventually to check
that it properly enumerates, though I suspect you've already done
quite extensive testing yourself.

Thanks for doing this!

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver

[Thierry Reding]

On Tue, Oct 17, 2017 at 03:13:54PM -0500, Rob Herring wrote:
[...]
> > diff --git 
> > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt 
> > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
[...]
> > +- resets : Must contain an entry for each entry in reset-names.
> > +  See ../reset/reset.txt for details.
> > +- reset-names : Must include the following entries:
> > +  - vde
> 
> -names is pointless when there is only one.

I'd prefer to keep it. In the past we occasionally had to add clocks or
resets to a device tree node where only one had been present (and hence
no -names property) and that caused some awkwardness because verbiage
had to be added to the bindings that clarified that one particular entry
(the original one) always had to come first.

Thierry


signature.asc
Description: PGP signature

Re: [PATCHv4 0/4] tegra-cec: add Tegra HDMI CEC support

[Thierry Reding]

On Sat, Oct 14, 2017 at 02:08:31PM +0200, Hans Verkuil wrote:
> Hi Thierry,
> 
> On 09/11/2017 02:29 PM, Hans Verkuil wrote:
> > From: Hans Verkuil 
> > 
> > This patch series adds support for the Tegra CEC functionality.
> > 
> > This v4 has been rebased to the latest 4.14 pre-rc1 mainline.
> > 
> > Please review! Other than for the bindings that are now Acked I have not
> > received any feedback.
> 
> Can you or someone else from the Tegra maintainers review this?
> 
> I have not heard anything about this patch series, nor of the previous
> versions of this series. What's the hold-up?

Sorry about that. I've been meaning to look at this for a while now, but
never got around to it. From a quick glance this looks good. Let me take
this for a quick test-drive when I'm back at the office next week and
I'll report back.

Is there any particular ordering that we need to observe in order to
merge this? Looks to me like it would be safe to merge patches 1 and 3
through the CEC (media?) tree and take the others through DRM and Tegra
separately without breaking anything.

Thierry

> > The first patch documents the CEC bindings, the second adds support
> > for this to tegra124.dtsi and enables it for the Jetson TK1.
> > 
> > The third patch adds the CEC driver itself and the final patch adds
> > the cec notifier support to the drm/tegra driver in order to notify
> > the CEC driver whenever the physical address changes.
> > 
> > I expect that the dts changes apply as well to the Tegra X1/X2 and possibly
> > other Tegra SoCs, but I can only test this with my Jetson TK1 board.
> > 
> > The dt-bindings and the tegra-cec driver would go in through the media
> > subsystem, the drm/tegra part through the drm subsystem and the dts
> > changes through (I guess) the linux-tegra developers. Luckily they are
> > all independent of one another.
> > 
> > To test this you need the CEC utilities from 
> > git://linuxtv.org/v4l-utils.git.
> > 
> > To build this:
> > 
> > git clone git://linuxtv.org/v4l-utils.git
> > cd v4l-utils
> > ./bootstrap.sh; ./configure
> > make
> > sudo make install # optional, you really only need utils/cec*
> > 
> > To test:
> > 
> > cec-ctl --playback # configure as playback device
> > cec-ctl -S # detect all connected CEC devices
> > 
> > See here for the public CEC API:
> > 
> > https://hverkuil.home.xs4all.nl/spec/uapi/cec/cec-api.html
> > 
> > Regards,
> > 
> > Hans
> > 
> > Changes since v3:
> > 
> > - Use the new CEC_CAP_DEFAULTS define
> > - Use IS_ERR(cec->adap) instead of IS_ERR_OR_NULL(cec->adap)
> >   (cec_allocate_adapter never returns a NULL pointer)
> > - Drop the device_init_wakeup: wakeup is not (yet) supported by
> >   the CEC framework and I have never tested it.
> > 
> > Hans Verkuil (4):
> >   dt-bindings: document the tegra CEC bindings
> >   ARM: tegra: add CEC support to tegra124.dtsi
> >   tegra-cec: add Tegra HDMI CEC driver
> >   drm/tegra: add cec-notifier support
> > 
> >  .../devicetree/bindings/media/tegra-cec.txt|  27 ++
> >  MAINTAINERS|   8 +
> >  arch/arm/boot/dts/tegra124-jetson-tk1.dts  |   4 +
> >  arch/arm/boot/dts/tegra124.dtsi|  12 +-
> >  drivers/gpu/drm/tegra/Kconfig  |   1 +
> >  drivers/gpu/drm/tegra/drm.h|   3 +
> >  drivers/gpu/drm/tegra/hdmi.c   |   9 +
> >  drivers/gpu/drm/tegra/output.c |   6 +
> >  drivers/media/platform/Kconfig |  11 +
> >  drivers/media/platform/Makefile|   2 +
> >  drivers/media/platform/tegra-cec/Makefile  |   1 +
> >  drivers/media/platform/tegra-cec/tegra_cec.c   | 501 
> > +
> >  drivers/media/platform/tegra-cec/tegra_cec.h   | 127 ++
> >  13 files changed, 711 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/tegra-cec.txt
> >  create mode 100644 drivers/media/platform/tegra-cec/Makefile
> >  create mode 100644 drivers/media/platform/tegra-cec/tegra_cec.c
> >  create mode 100644 drivers/media/platform/tegra-cec/tegra_cec.h
> > 
> 


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 2/5] pwm: omap-dmtimer: Allow for setting dmtimer clock source

[Thierry Reding]

On Wed, Jun 22, 2016 at 10:22:18PM +0300, Ivaylo Dimitrov wrote:
> OMAP GP timers can have different input clocks that allow different PWM
> frequencies. However, there is no other way of setting the clock source but
> through clocks or clock-names properties of the timer itself. This limits
> PWM functionality to only the frequencies allowed by the particular clock
> source. Allowing setting the clock source by PWM rather than by timer
> allows different PWMs to have different ranges by not hard-wiring the clock
> source to the timer.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov...@gmail.com>
> Acked-by: Rob Herring <r...@kernel.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt |  4 
>  drivers/pwm/pwm-omap-dmtimer.c | 12 +++-
>  2 files changed, 11 insertions(+), 5 deletions(-)

Acked-by: Thierry Reding <tred...@nvidia.com>


signature.asc
Description: PGP signature

[PATCH] [media] uvcvideo: Fix build if !MEDIA_CONTROLLER

[Thierry Reding]

From: Thierry Reding <tred...@nvidia.com>

Accesses to the UVC device's mdev field need to be protected by a
preprocessor conditional to avoid build errors, since the field is only
included if the MEDIA_CONTROLLER option is selected.

Fixes: 1590ad7b5271 ("[media] media-device: split media initialization and 
registration")
Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
Cc: Mauro Carvalho Chehab <mche...@osg.samsung.com>
Signed-off-by: Thierry Reding <tred...@nvidia.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index 4dfd3eb814e7..fc63f9aae63e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1937,9 +1937,11 @@ static int uvc_probe(struct usb_interface *intf,
if (uvc_register_chains(dev) < 0)
goto error;
 
+#ifdef CONFIG_MEDIA_CONTROLLER
/* Register the media device node */
if (media_device_register(>mdev) < 0)
goto error;
+#endif
 
/* Save our data pointer in the interface data. */
usb_set_intfdata(intf, dev);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Thierry Reding]

On Mon, Sep 21, 2015 at 11:55:53AM -0700, Bryan Wu wrote:
[...]
> +static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> + struct tegra_csi_device *csi = to_csi(subdev);
> + struct tegra_channel *chan = subdev->host_priv;
> + enum tegra_csi_port_num port_num = (chan->port & 1) ? PORT_B : PORT_A;
> + struct tegra_csi_port *port = >ports[port_num];
> + int ret;
> +
> + if (enable) {
[...]
> + } else {
> + u32 val = pp_read(port, TEGRA_CSI_PIXEL_PARSER_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_PIXEL_PARSER_STATUS 0x%08x\n", 
> val);
> +
> + val = cil_read(port, TEGRA_CSI_CIL_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_CIL_STATUS 0x%08x\n", val);
> +
> + val = cil_read(port, TEGRA_CSI_CILX_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_CILX_STATUS 0x%08x\n", val);
> + 

I was going to apply this and give it a spin, but then git am complained
about trailing whitespace above...

> +#ifdef DEBUG
> + val = csi_read(csi, TEGRA_CSI_DEBUG_COUNTER_0);
> + dev_err(>dev, "TEGRA_CSI_DEBUG_COUNTER_0 0x%08x\n", val);
> +#endif
> +
> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
> +  (0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
> +  CSI_PP_DISABLE);
> +
> + clk_disable_unprepare(csi->clk);
> + }
> + 

and here, ...

> +static int tegra_csi_probe(struct platform_device *pdev)
> +{
[...]
> + for (i = 0; i < TEGRA_CSI_PORTS_NUM; i++) {
> + /* Initialize the default format */
> + csi->ports[i].format.code = TEGRA_VF_DEF;
> + csi->ports[i].format.field = V4L2_FIELD_NONE;
> + csi->ports[i].format.colorspace = V4L2_COLORSPACE_SRGB;
> + csi->ports[i].format.width = TEGRA_DEF_WIDTH;
> + csi->ports[i].format.height = TEGRA_DEF_HEIGHT;
> +
> + /* Initialize port register bases */
> + csi->ports[i].pixel_parser = csi->iomem +
> +  (i & 1) * TEGRA_CSI_PORT_OFFSET;
> + csi->ports[i].cil = csi->iomem + TEGRA_CSI_CIL_OFFSET + 

here and...

> + (i & 1) * TEGRA_CSI_PORT_OFFSET;
> + csi->ports[i].tpg = csi->iomem + TEGRA_CSI_TPG_OFFSET + 

here.

Might be worth fixing those up if you'll respin anyway.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 3/3] Documentation: DT bindings: add VI and CSI bindings

[Thierry Reding]

On Mon, Sep 21, 2015 at 11:55:55AM -0700, Bryan Wu wrote:
> Signed-off-by: Bryan Wu 
> ---
>  .../bindings/gpu/nvidia,tegra20-host1x.txt | 211 
> -
>  1 file changed, 205 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt 
> b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
[...]
> +  - ports: 2 ports presenting 2 channels of CSI. Each port has 2 endpoints:
> +one connects to sensor device tree node as input and the other one 
> connects
> +to VI endpoint.

It looks to me like we'll only need the first of these endpoints. The
connection to the VI is implied. Even more so if the CSI nodes are moved
into the VI node as children (which I really think they should be).

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 2/3] ARM64: add tegra-vi support in T210 device-tree

[Thierry Reding]

Hi Bryan,

This patchset really needs to be Cc'ed to linux-te...@vger.kernel.org,
it's becoming impossible to track it otherwise.

On Mon, Sep 21, 2015 at 11:55:54AM -0700, Bryan Wu wrote:
[...]
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi 
> b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index 1168bcd..3f6501f 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -109,9 +109,181 @@
>  
>   vi@0,5408 {
>   compatible = "nvidia,tegra210-vi";
> - reg = <0x0 0x5408 0x0 0x0004>;
> + reg = <0x0 0x5408 0x0 0x800>;

This now no longer matches the address map in the TRM.

>   interrupts = ;
>   status = "disabled";
> + clocks = <_car TEGRA210_CLK_VI>,
> +  <_car TEGRA210_CLK_CSI>,
> +  <_car TEGRA210_CLK_PLL_C>;
> + clock-names = "vi", "csi", "parent";
> + resets = <_car 20>;
> + reset-names = "vi";
> +
> + power-domains = < TEGRA_POWERGATE_VENC>;

As an aside, this will currently prevent the driver from probing because
the generic power domain code will return -EPROBE_DEFER if this property
is encountered but the corresponding driver (for the PMC) hasn't
registered any power domains yet. So until the PMC driver changes have
been merged we can't add these power-domains properties.

That's not something for you to worry about, though. I'll make sure to
strip this out if it happens to get merged before the PMC driver changes
and add it back it afterwards.

> +
> + iommus = < TEGRA_SWGROUP_VI>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + vi_in0: endpoint {
> + remote-endpoint = <_out0>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> +
> + vi_in1: endpoint {
> + remote-endpoint = <_out1>;
> + };
> + };
> + port@2 {
> + reg = <2>;
> +
> + vi_in2: endpoint {
> + remote-endpoint = <_out2>;
> + };
> + };
> + port@3 {
> + reg = <3>;
> +
> + vi_in3: endpoint {
> + remote-endpoint = <_out3>;
> + };
> + };
> + port@4 {
> + reg = <4>;
> +
> + vi_in4: endpoint {
> + remote-endpoint = <_out4>;
> + };
> + };
> + port@5 {
> + reg = <5>;
> +
> + vi_in5: endpoint {
> + remote-endpoint = <_out5>;
> + };
> + };
> +
> + };
> + };
> +
> + csi@0,54080838 {

I think this and its siblings should be children of the vi node. They
are within the same memory aperture according to the TRM and the fact
that the VI needs to reference the "CSI" clock indicates that the
coupling is tighter than this current DT layout makes it out to be.

> + compatible = "nvidia,tegra210-csi";
> + reg = <0x0 0x54080838 0x0 0x700>;

Some of the internal register documentation indicates that the register
range actually starts at an offset of 0x800, it just so happens that we
don't use any of the registers from 0x800 to 0x837. So I think this
needs to be adapted.

> + clocks = <_car TEGRA210_CLK_CILAB>;
> + clock-names = "cil";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + csi_in0: 

Re: [PATCH 3/3] Documentation: DT bindings: add VI and CSI bindings

[Thierry Reding]

On Mon, Sep 21, 2015 at 11:55:55AM -0700, Bryan Wu wrote:
> Signed-off-by: Bryan Wu 
> ---
>  .../bindings/gpu/nvidia,tegra20-host1x.txt | 211 
> -
>  1 file changed, 205 insertions(+), 6 deletions(-)

Also you probably want to include devicet...@vger.kernel.org on the
binding patch to give people the chance to review.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Thierry Reding]

On Tue, Aug 25, 2015 at 04:15:58PM +0200, Hans Verkuil wrote:
 On 08/25/15 15:44, Thierry Reding wrote:
  On Mon, Aug 24, 2015 at 05:26:20PM -0700, Bryan Wu wrote:
[...]
   For CMA we need increase the default memory size.
  
  I'd rather not rely on CMA at all, especially since we do have a way
  around it.
 
 For the record, I have no problem with it if we start out with contiguous
 DMA now and enhance it later. I get the impression that getting the IOMMU
 to work is non-trivial, and I don't think it should block merging of this
 driver.
 
 This is all internal to the driver, so changing it later will not affect
 userspace.

Fair enough.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Thierry Reding]

On Tue, Aug 25, 2015 at 08:30:41AM +0200, Hans Verkuil wrote:
 A quick follow-up to Thierry's excellent review:
 
 On 08/25/2015 02:26 AM, Bryan Wu wrote:
  On 08/21/2015 06:03 AM, Thierry Reding wrote:
  On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote:
 
 snip
 
  +static void
  +__tegra_channel_try_format(struct tegra_channel *chan, struct 
  v4l2_pix_format *pix,
  +   const struct tegra_video_format **fmtinfo)
  +{
  + const struct tegra_video_format *info;
  + unsigned int min_width;
  + unsigned int max_width;
  + unsigned int min_bpl;
  + unsigned int max_bpl;
  + unsigned int width;
  + unsigned int align;
  + unsigned int bpl;
  +
  + /* Retrieve format information and select the default format if the
  +  * requested format isn't supported.
  +  */
  + info = tegra_core_get_format_by_fourcc(pix-pixelformat);
  + if (!info)
  + info = tegra_core_get_format_by_fourcc(TEGRA_VF_DEF_FOURCC);
  Should this not be an error? As far as I can tell this is silently
  substituting the default format for the requested one if the requested
  one isn't supported. Isn't the whole point of this to find out if some
  format is supported?
 
  I think it should return some error and escape following code. I will 
  fix that.
 
 Actually, this code is according to the V4L2 spec: if the given format is
 not supported, then VIDIOC_TRY_FMT should replace it with a valid default
 format.
 
 The reality is a bit more complex: in many drivers this was never reviewed
 correctly and we ended up with some drivers that return an error for this
 case and some drivers that follow the spec. Historically TV capture drivers
 return an error, webcam drivers don't. Most unfortunate.
 
 Since this driver is much more likely to be used with sensors I would
 follow the spec here and substitute an invalid format with a default
 format.

Okay, sounds good to me.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Thierry Reding]

On Mon, Aug 24, 2015 at 05:26:20PM -0700, Bryan Wu wrote:
 On 08/21/2015 06:03 AM, Thierry Reding wrote:
 On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote:
[...]
 +#define TEGRA_CSI_PHY_CIL_COMMAND  0x0908
 This doesn't seem to be used at all.
 
 Actually this PHY register just has this one only, I need define it as 0x0
 offset here. Let's keep this since in future we might have more PHY
 registers.

Yes, I had been wondering about the PHY registers. If we make this a
register with offset 0, as I understand it will become used because of
the phy_{readl,writel}() rework.

 +#define TEGRA_CSI_PATTERN_GENERATOR_CTRL   0x000
 +#define TEGRA_CSI_PG_BLANK 0x004
 +#define TEGRA_CSI_PG_PHASE 0x008
 +#define TEGRA_CSI_PG_RED_FREQ  0x00c
 +#define TEGRA_CSI_PG_RED_FREQ_RATE 0x010
 +#define TEGRA_CSI_PG_GREEN_FREQ0x014
 +#define TEGRA_CSI_PG_GREEN_FREQ_RATE   0x018
 +#define TEGRA_CSI_PG_BLUE_FREQ 0x01c
 +#define TEGRA_CSI_PG_BLUE_FREQ_RATE0x020
 +#define TEGRA_CSI_PG_AOHDR 0x024
 +
 +#define TEGRA_CSI_DPCM_CTRL_A  0xad0
 +#define TEGRA_CSI_DPCM_CTRL_B  0xad4
 +#define TEGRA_CSI_STALL_COUNTER0xae8
 +#define TEGRA_CSI_CSI_READONLY_STATUS  0xaec
 +#define TEGRA_CSI_CSI_SW_STATUS_RESET  0xaf0
 +#define TEGRA_CSI_CLKEN_OVERRIDE   0xaf4
 +#define TEGRA_CSI_DEBUG_CONTROL0xaf8
 +#define TEGRA_CSI_DEBUG_COUNTER_0  0xafc
 +#define TEGRA_CSI_DEBUG_COUNTER_1  0xb00
 +#define TEGRA_CSI_DEBUG_COUNTER_2  0xb04
 Some of these are unused. I guess there's an argument to be made to
 include all register definitions rather than just the used ones, if for
 nothing else than completeness. I'll defer to Hans's judgement on this.
 
 These are VI/CSI global registers shared by all the channels. Some of them
 are used in this driver, I suggest we keep them here.

Fine with me.

 +{
 +   if (chan-bypass)
 +   return;
 I don't see this being set anywhere. Is it dead code? Also the only
 description I see is that it's used to bypass register writes, but I
 don't see an explanation why that's necessary.
 
 We are unifying our downstream VI driver with V4L2 VI driver. And this
 upstream work is the first step to help that.
 
 We are also backporting this driver back to our internal 3.10 kernel which
 is using nvhost channel to submit register operations from userspace to
 host1x and VI hardware. Then in this case, our driver needs to bypass all
 the register operations otherwise we got conflicts between these 2 paths.
 
 That's why I put bypass mode here. And bypass mode can be set in device tree
 or from v4l2-ctrls.

I don't think it's fair to burden upstream with code that will only ever
be used downstream. Let's split these changes into a separate patch that
can be carried downstream.

 +/* Syncpoint bits of TEGRA_VI_CFG_VI_INCR_SYNCPT */
 +static u32 sp_bit(struct tegra_channel *chan, u32 sp)
 +{
 +   return (sp + chan-port * 4)  8;
 +}
 Technically this returns a mask, not a bit, so sp_mask() would be more
 appropriate.
 Actually it returns the syncpoint value for each port not a mask. Probably
 sp_bits() is better.

Looking at the TRM, the field that this generates a value for is called
VI_COND (in the VI_CFG_VI_INCR_SYNCPT register), so perhaps this should
really be a macro and named something like:

#define VI_CFG_VI_INCR_SYNCPT_COND(x) (((x)  0xff)  8)

As for the arithmetic, that doesn't seem to match up. Quoting from your
original patch:

   +/* VI registers */
   +#define TEGRA_VI_CFG_VI_INCR_SYNCPT 0x000
   +#define  SP_PP_LINE_START4
   +#define  SP_PP_FRAME_START   5
   +#define  SP_MW_REQ_DONE  6
   +#define  SP_MW_ACK_DONE  7

This doesn't seem to match the TRM, which has the following values:

 0 = IMMEDIATE
 1 = OP_DONE
 2 = RD_DONE
 3 = REG_WR_SAFE
 4 = VI_MWA_REQ_DONE
 5 = VI_MWB_REQ_DONE
 6 = VI_MWA_ACK_DONE
 7 = VI_MWB_ACK_DONE
 8 = VI_ISPA_DONE
 9 = VI_CSI_PPA_FRAME_START
10 = VI_CSI_PPB_FRAME_START
11 = VI_CSI_PPA_LINE_START
12 = VI_CSI_PPB_LINE_START
13 = VI_VGP0_RCVD
14 = VI_VGP1_RCVD
15 = VI_ISPB_DONE

Comparing with the internal register manuals it looks like the TRM is
actually wrong. Can you file an internal bug to rectify this and Cc me
on it, please?

Irrespective, since this is generating content for a register field it
would seem more consistent to define

Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Thierry Reding]

On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote:
 NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
 controller which can support up to 6 MIPI CSI camera sensors.
 
 This patch adds a V4L2 media controller and capture driver to support
 Tegra VI hardware. It's verified with Tegra built-in test pattern
 generator.

Hi Bryan,

I've been looking forward to seeing this posted. I don't know the VI
hardware in very much detail, nor am I an expert on the media framework,
so I will primarily comment on architectural or SoC-specific things.

By the way, please always Cc linux-te...@vger.kernel.org on all patches
relating to Tegra. That way people not explicitly Cc'ed but still
interested in Tegra will see this code, even if they aren't subscribed
to the linux-media mailing list.

 Signed-off-by: Bryan Wu pe...@nvidia.com
 Reviewed-by: Hans Verkuil hans.verk...@cisco.com
 ---
  drivers/media/platform/Kconfig   |1 +
  drivers/media/platform/Makefile  |2 +
  drivers/media/platform/tegra/Kconfig |9 +
  drivers/media/platform/tegra/Makefile|3 +
  drivers/media/platform/tegra/tegra-channel.c | 1074 
 ++
  drivers/media/platform/tegra/tegra-core.c|  295 +++
  drivers/media/platform/tegra/tegra-core.h|  134 
  drivers/media/platform/tegra/tegra-vi.c  |  585 ++
  drivers/media/platform/tegra/tegra-vi.h  |  224 ++
  include/dt-bindings/media/tegra-vi.h |   35 +
  10 files changed, 2362 insertions(+)
  create mode 100644 drivers/media/platform/tegra/Kconfig
  create mode 100644 drivers/media/platform/tegra/Makefile
  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
  create mode 100644 drivers/media/platform/tegra/tegra-core.c
  create mode 100644 drivers/media/platform/tegra/tegra-core.h
  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
  create mode 100644 drivers/media/platform/tegra/tegra-vi.h
  create mode 100644 include/dt-bindings/media/tegra-vi.h

I can't spot a device tree binding document for this, but we'll need one
to properly review this driver.

 diff --git a/drivers/media/platform/tegra/Kconfig 
 b/drivers/media/platform/tegra/Kconfig
 new file mode 100644
 index 000..a69d1b2
 --- /dev/null
 +++ b/drivers/media/platform/tegra/Kconfig
 @@ -0,0 +1,9 @@
 +config VIDEO_TEGRA
 + tristate NVIDIA Tegra Video Input Driver (EXPERIMENTAL)

I don't think the (EXPERIMENTAL) is warranted. Either the driver works
or it doesn't. And I assume you already tested that it works, even if
only using the TPG.

 + depends on VIDEO_V4L2  VIDEO_V4L2_SUBDEV_API  OF

This seems to be missing a couple of dependencies. For example I would
expect at least TEGRA_HOST1X to be listed here to make sure people can't
select this when the host1x API isn't available. I would also expect
some sort of architecture dependency because it really makes no sense to
build this if Tegra isn't supported.

If you are concerned about compile coverage you can make that explicit
using a COMPILE_TEST alternative such as:

depends on ARCH_TEGRA || (ARM  COMPILE_TEST)

Note that the ARM dependency in there makes sure that HAVE_IOMEM is
selected, so this could also be:

depends on ARCH_TEGRA || (HAVE_IOMEM  COMPILE_TEST)

though that'd still leave open the possibility of build breakage because
of some missing support.

If you add the dependency on TEGRA_HOST1X that I mentioned above you
shouldn't need any architecture dependency because TEGRA_HOST1X implies
those already.

 + select VIDEOBUF2_DMA_CONTIG
 + ---help---
 +   Driver for Video Input (VI) device controller in NVIDIA Tegra SoC.

I'd reword this slightly as:

  Driver for the Video Input (VI) controller found on NVIDIA Tegra
  SoCs.

 +
 +   TO compile this driver as a module, choose M here: the module will be

s/TO/To/.

 +   called tegra-video.

 diff --git a/drivers/media/platform/tegra/Makefile 
 b/drivers/media/platform/tegra/Makefile
 new file mode 100644
 index 000..c8eff0b
 --- /dev/null
 +++ b/drivers/media/platform/tegra/Makefile
 @@ -0,0 +1,3 @@
 +tegra-video-objs += tegra-core.o tegra-vi.o tegra-channel.o

I'd personally leave out the redundant tegra- prefix here, because the
files are in a tegra/ subdirectory already.

 +obj-$(CONFIG_VIDEO_TEGRA) += tegra-video.o
 diff --git a/drivers/media/platform/tegra/tegra-channel.c 
 b/drivers/media/platform/tegra/tegra-channel.c
 new file mode 100644
 index 000..b0063d2
 --- /dev/null
 +++ b/drivers/media/platform/tegra/tegra-channel.c
 @@ -0,0 +1,1074 @@
 +/*
 + * NVIDIA Tegra Video Input Device
 + *
 + * Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
 + *
 + * Author: Bryan Wu pe...@nvidia.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.

Re: [RFC] How implement Secure Data Path ?

[Thierry Reding]

On Wed, May 06, 2015 at 03:15:32PM +0200, Daniel Vetter wrote:
 On Wed, May 06, 2015 at 11:19:21AM +0200, Thierry Reding wrote:
  On Wed, May 06, 2015 at 10:35:52AM +0200, Daniel Vetter wrote:
   On Tue, May 05, 2015 at 05:54:05PM +0100, One Thousand Gnomes wrote:
 First what is Secure Data Path ? SDP is a set of hardware features to 
 garanty
 that some memories regions could only be read and/or write by 
 specific hardware
 IPs. You can imagine it as a kind of memory firewall which 
 grant/revoke
 accesses to memory per devices. Firewall configuration must be done 
 in a trusted
 environment: for ARM architecture we plan to use OP-TEE + a trusted
 application to do that.

It's not just an ARM feature so any basis for this in the core code
should be generic, whether its being enforced by ARM SDP, various Intel
feature sets or even via a hypervisor.

 I have try 2 hacky approachs with dma_buf:
 - add a secure field in dma_buf structure and configure firewall in
   dma_buf_{map/unmap}_attachment() functions.

How is SDP not just another IOMMU. The only oddity here is that it
happens to configure buffers the CPU can't touch and it has a control
mechanism that is designed to cover big media corp type uses where the
threat model is that the system owner is the enemy. Why does anything 
care
about it being SDP, there are also generic cases this might be a useful
optimisation (eg knowing the buffer isn't CPU touched so you can 
optimise
cache flushing).

The control mechanism is a device/platform detail as with any IOMMU. It
doesn't matter who configures it or how, providing it happens.

We do presumably need some small core DMA changes - anyone trying to map
such a buffer into CPU space needs to get a warning or error but what
else ?

 From buffer allocation point of view I also facing a problem because 
 when v4l2
 or drm/kms are exporting buffers by using dma_buf they don't attaching
 themself on it and never call dma_buf_{map/unmap}_attachment(). This 
 is not
 an issue in those framework while it is how dma_buf exporters are
 supposed to work.

Which could be addressed if need be.

So if SDP is just another IOMMU feature, just as stuff like IMR is on
some x86 devices, and hypervisor enforced protection is on assorted
platforms why do we need a special way to do it ? Is there anything
actually needed beyond being able to tell the existing DMA code that 
this
buffer won't be CPU touched and wiring it into the DMA operations for 
the
platform ?
   
   Iirc most of the dma api stuff gets unhappy when memory isn't struct page
   backed. In i915 we do use sg tables everywhere though (even for memory not
   backed by struct page, e.g. the stolen range the bios prereserves), but
   we fill those out manually.
   
   A possible generic design I see is to have a secure memory allocator
   device which doesn nothing else but hand out dma-bufs.
  
  Are you suggesting a device file with a custom set of IOCTLs for this?
  Or some in-kernel API that would perform the secure allocations? I
  suspect the former would be better suited, because it gives applications
  the control over whether they need secure buffers or not. The latter
  would require custom extensions in every driver to make them allocate
  from a secure memory pool.
 
 Yes the idea would be a special-purpose allocater thing like ion. Might
 even want that to be a syscall to do it properly.

Would you care to elaborate why a syscall would be more proper? Not that
I'm objecting to it, just for my education.

  For my understanding, would the secure memory allocator be responsible
  for setting up the permissions to access the memory at attachment time?
 
 Well not permission checks, but hw capability checks. The allocator should
 have platform knowledge about which devices can access such secure memory
 (since some will definitely not be able to do that for fear of just plain
 sending it out to the world).

At least on Tegra there are controls to grant access to the VPR to a
given device, so I'd expect some driver needing to frob some registers
before the device can access a secure buffer.

Thierry


pgp5aM_ddH97M.pgp
Description: PGP signature

Re: [RFC] How implement Secure Data Path ?

[Thierry Reding]

On Wed, May 06, 2015 at 07:29:56AM -0400, Rob Clark wrote:
 On Wed, May 6, 2015 at 4:35 AM, Daniel Vetter dan...@ffwll.ch wrote:
  On Tue, May 05, 2015 at 05:54:05PM +0100, One Thousand Gnomes wrote:
   First what is Secure Data Path ? SDP is a set of hardware features to 
   garanty
   that some memories regions could only be read and/or write by specific 
   hardware
   IPs. You can imagine it as a kind of memory firewall which grant/revoke
   accesses to memory per devices. Firewall configuration must be done in a 
   trusted
   environment: for ARM architecture we plan to use OP-TEE + a trusted
   application to do that.
 
  It's not just an ARM feature so any basis for this in the core code
  should be generic, whether its being enforced by ARM SDP, various Intel
  feature sets or even via a hypervisor.
 
   I have try 2 hacky approachs with dma_buf:
   - add a secure field in dma_buf structure and configure firewall in
 dma_buf_{map/unmap}_attachment() functions.
 
  How is SDP not just another IOMMU. The only oddity here is that it
  happens to configure buffers the CPU can't touch and it has a control
  mechanism that is designed to cover big media corp type uses where the
  threat model is that the system owner is the enemy. Why does anything care
  about it being SDP, there are also generic cases this might be a useful
  optimisation (eg knowing the buffer isn't CPU touched so you can optimise
  cache flushing).
 
  The control mechanism is a device/platform detail as with any IOMMU. It
  doesn't matter who configures it or how, providing it happens.
 
  We do presumably need some small core DMA changes - anyone trying to map
  such a buffer into CPU space needs to get a warning or error but what
  else ?
 
   From buffer allocation point of view I also facing a problem because 
   when v4l2
   or drm/kms are exporting buffers by using dma_buf they don't attaching
   themself on it and never call dma_buf_{map/unmap}_attachment(). This is 
   not
   an issue in those framework while it is how dma_buf exporters are
   supposed to work.
 
  Which could be addressed if need be.
 
  So if SDP is just another IOMMU feature, just as stuff like IMR is on
  some x86 devices, and hypervisor enforced protection is on assorted
  platforms why do we need a special way to do it ? Is there anything
  actually needed beyond being able to tell the existing DMA code that this
  buffer won't be CPU touched and wiring it into the DMA operations for the
  platform ?
 
  Iirc most of the dma api stuff gets unhappy when memory isn't struct page
  backed. In i915 we do use sg tables everywhere though (even for memory not
  backed by struct page, e.g. the stolen range the bios prereserves), but
  we fill those out manually.
 
  A possible generic design I see is to have a secure memory allocator
  device which doesn nothing else but hand out dma-bufs. With that we can
  hide the platform-specific allocation methods in there (some need to
  allocate from carveouts, other just need to mark the pages specifically).
  Also dma-buf has explicit methods for cpu access, which are allowed to
  fail. And using the dma-buf attach tracking we can also reject dma to
  devices which cannot access the secure memory. Given all that I think
  going through the dma-buf interface but with a special-purpose allocator
  seems to fit.
 
  I'm not sure whether a special iommu is a good idea otoh: I'd expect that
  for most devices the driver would need to decide about which iommu to pick
  (or maybe keep track of some special flags for an extended dma_map
  interface). At least looking at gpu drivers using iommus would require
  special code, whereas fully hiding all this behind the dma-buf interface
  should fit in much better.
 
 
 jfwiw, I'd fully expect devices to be dealing with a mix of secure and
 insecure buffers, so I'm also not really sure how the 'special iommu'
 plan would play out..
 
 I think 'secure' allocator device sounds attractive from PoV of
 separating out platform nonsense.. not sure if it is exactly that
 easy, since importing device probably needs to set some special bits
 here and there..

I would expect there to be a central entity handing out the secure
buffers and that the entity would have the ability to grant access to
the buffers to devices. So I'm thinking that the exporter would deal
with this in the -attach() operation. That's passed a struct device,
so we should be able to retrieve the information necessary somehow.

Then again, maybe things will be more involved than that. I think the
way this typically works in consumer devices is that you need to jump
into secure firmware to get access granted. For example on Tegra most
registers that control this are TrustZone-protected, so if you don't
happen to be lucky enough to be running the kernel in secure mode you
can't enable access to secure memory from the kernel.

As Alan mentioned before this is designed with the assumption that the
user is not to be 

Re: [RFC] How implement Secure Data Path ?

[Thierry Reding]

On Wed, May 06, 2015 at 10:35:52AM +0200, Daniel Vetter wrote:
 On Tue, May 05, 2015 at 05:54:05PM +0100, One Thousand Gnomes wrote:
   First what is Secure Data Path ? SDP is a set of hardware features to 
   garanty
   that some memories regions could only be read and/or write by specific 
   hardware
   IPs. You can imagine it as a kind of memory firewall which grant/revoke
   accesses to memory per devices. Firewall configuration must be done in a 
   trusted
   environment: for ARM architecture we plan to use OP-TEE + a trusted
   application to do that.
  
  It's not just an ARM feature so any basis for this in the core code
  should be generic, whether its being enforced by ARM SDP, various Intel
  feature sets or even via a hypervisor.
  
   I have try 2 hacky approachs with dma_buf:
   - add a secure field in dma_buf structure and configure firewall in
 dma_buf_{map/unmap}_attachment() functions.
  
  How is SDP not just another IOMMU. The only oddity here is that it
  happens to configure buffers the CPU can't touch and it has a control
  mechanism that is designed to cover big media corp type uses where the
  threat model is that the system owner is the enemy. Why does anything care
  about it being SDP, there are also generic cases this might be a useful
  optimisation (eg knowing the buffer isn't CPU touched so you can optimise
  cache flushing).
  
  The control mechanism is a device/platform detail as with any IOMMU. It
  doesn't matter who configures it or how, providing it happens.
  
  We do presumably need some small core DMA changes - anyone trying to map
  such a buffer into CPU space needs to get a warning or error but what
  else ?
  
   From buffer allocation point of view I also facing a problem because 
   when v4l2
   or drm/kms are exporting buffers by using dma_buf they don't attaching
   themself on it and never call dma_buf_{map/unmap}_attachment(). This is 
   not
   an issue in those framework while it is how dma_buf exporters are
   supposed to work.
  
  Which could be addressed if need be.
  
  So if SDP is just another IOMMU feature, just as stuff like IMR is on
  some x86 devices, and hypervisor enforced protection is on assorted
  platforms why do we need a special way to do it ? Is there anything
  actually needed beyond being able to tell the existing DMA code that this
  buffer won't be CPU touched and wiring it into the DMA operations for the
  platform ?
 
 Iirc most of the dma api stuff gets unhappy when memory isn't struct page
 backed. In i915 we do use sg tables everywhere though (even for memory not
 backed by struct page, e.g. the stolen range the bios prereserves), but
 we fill those out manually.
 
 A possible generic design I see is to have a secure memory allocator
 device which doesn nothing else but hand out dma-bufs.

Are you suggesting a device file with a custom set of IOCTLs for this?
Or some in-kernel API that would perform the secure allocations? I
suspect the former would be better suited, because it gives applications
the control over whether they need secure buffers or not. The latter
would require custom extensions in every driver to make them allocate
from a secure memory pool.

For my understanding, would the secure memory allocator be responsible
for setting up the permissions to access the memory at attachment time?

With that we can
 hide the platform-specific allocation methods in there (some need to
 allocate from carveouts, other just need to mark the pages specifically).
 Also dma-buf has explicit methods for cpu access, which are allowed to
 fail. And using the dma-buf attach tracking we can also reject dma to
 devices which cannot access the secure memory. Given all that I think
 going through the dma-buf interface but with a special-purpose allocator
 seems to fit.

That sounds like a flexible enough design to me. I think it's something
that we could easily implement on Tegra. The memory controller on Tegra
implements this using a special video-protect aperture (VPR) and memory
clients can individually be allowed access to this aperture. That means
VPR is a carveout that is typically set up by some secure firmware, and
that in turn, as I understand it, would imply we won't have struct page
pointers for the backing memory in this case either.

I suspect that it should work out fine to not require struct page backed
memory for this infrastructure since by definition the CPU won't be
allowed to access it anyway.

 I'm not sure whether a special iommu is a good idea otoh: I'd expect that
 for most devices the driver would need to decide about which iommu to pick
 (or maybe keep track of some special flags for an extended dma_map
 interface). At least looking at gpu drivers using iommus would require
 special code, whereas fully hiding all this behind the dma-buf interface
 should fit in much better.

As I understand it, even though the VPR on Tegra is a carveout it still
is 

Re: [PATCHv3 0/3] hdmi: add unpack and logging functions

[Thierry Reding]

On Fri, Dec 19, 2014 at 01:14:19PM +0100, Hans Verkuil wrote:
 This patch series adds new HDMI 2.0/CEA-861-F defines to hdmi.h and
 adds unpacking and logging functions to hdmi.c. It also uses those
 in the V4L2 adv7842 driver (and they will be used in other HDMI drivers
 once this functionality is merged).
 
 Changes since v2:
 - Applied most comments from Thierry's review
 - Renamed HDMI_AUDIO_CODING_TYPE_EXT_STREAM as per Thierry's suggestion.
 
 Thierry, if this OK, then please give your Ack and I'll post a pull
 request for 3.20 for the media git tree.

Patches 1, 2 and 3:

Acked-by: Thierry Reding tred...@nvidia.com


pgpULFGjIc2lj.pgp
Description: PGP signature

Re: FOSDEM15: Graphics DevRoom: call for speakers.

[Thierry Reding]

On Tue, Dec 09, 2014 at 03:39:26PM +0100, Luc Verhaegen wrote:
 On Thu, Oct 02, 2014 at 07:44:57PM +0200, Luc Verhaegen wrote:
  Hi,
  
  At FOSDEM on the 31st of january and the 1st of February 2015, there 
  will be another graphics DevRoom. URL: https://fosdem.org/2015/
 
  Slots will be handed out on a first come, first serve basis. The best 
  slots will go to those who apply the earliest. The amount of slots is 
  currently not known yet, but i expect there to be around 16 available (8 
  on each day), so act quickly.
 
  As for deadlines, i hope to have a pretty much complete schedule between 
  christmas and the new year. The rockhard printed schedule deadline is 
  probably January 9th, after that you will not be featured in the booklet 
  and you will have a lot less visitors. I will hopefully be able to lock 
  down entries and descriptions after that date.
 
 It's been more than 2 months since the original email, it's less than 
 two months away from the event, and one month away from what usually is 
 the deadline for the booklet. File your talk now, while there are still 
 some useful slots available.
 
 Also, for those who have filed already but who have left their abstracts 
 open, please get those filed in ASAP. Your talk will be only be ordered 
 in when at least the basics are provided.

Hi Luc,

I realize I'm terribly late, but it took quite some time to get travel
arranged. Looking at the schedule there still seem to be some free
slots. Does it make sense to still submit a talk? I was asked to give
one on atomic modesetting from a driver developer's perspective.

Thierry


pgpxMSAVrv7BR.pgp
Description: PGP signature

Re: [PATCHv2 2/3] hdmi: added unpack and logging functions for InfoFrames

[Thierry Reding]

On Tue, Dec 02, 2014 at 01:08:45PM +0100, Hans Verkuil wrote:
 From: Martin Bugge marbu...@cisco.com
 
 When receiving video it is very useful to be able to unpack the InfoFrames.
 Logging is useful as well, both for transmitters and receivers.
 
 Especially when implementing the VIDIOC_LOG_STATUS ioctl (supported by many
 V4L2 drivers) for a receiver it is important to be able to easily log what
 the InfoFrame contains. This greatly simplifies debugging.
 
 Signed-off-by: Martin Bugge marbu...@cisco.com
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 ---
  drivers/video/hdmi.c | 819 
 ++-
  include/linux/hdmi.h |   4 +
  2 files changed, 816 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
 index 9e758a8..5f7ab47 100644
 --- a/drivers/video/hdmi.c
 +++ b/drivers/video/hdmi.c
 @@ -27,10 +27,12 @@
  #include linux/export.h
  #include linux/hdmi.h
  #include linux/string.h
 +#include linux/device.h
  
 -static void hdmi_infoframe_checksum(void *buffer, size_t size)
 +#define hdmi_log(fmt, ...) dev_printk(level, dev, fmt, ##__VA_ARGS__)

I personally dislike macros like these that make assumptions about the
environment. While somewhat longer, directly using dev_printk() would in
my opinion be clearer.

But I realize this is somewhat bikesheddy, so don't consider it a hard
objection.

 +
 +static u8 hdmi_infoframe_checksum(u8 *ptr, size_t size)
  {
 - u8 *ptr = buffer;

For consistency with the other functions I'd prefer this to take void *
instead of u8 *. That'd also clean up the diff in this part a little.

   u8 csum = 0;
   size_t i;
  
 @@ -38,7 +40,14 @@ static void hdmi_infoframe_checksum(void *buffer, size_t 
 size)
   for (i = 0; i  size; i++)
   csum += ptr[i];
  
 - ptr[3] = 256 - csum;
 + return 256 - csum;
 +}
 +
 +static void hdmi_infoframe_set_checksum(void *buffer, size_t size)
 +{
 + u8 *ptr = buffer;
 + /* update checksum */

I think checkpatch warns these days about missing blank lines after the
declaration block. But perhaps it is tricked by the comment immediately
following.

Nit: I don't think the comment adds any value.

 +static void hdmi_infoframe_log_header(const char *level,
 +   struct device *dev, void *f)

Perhaps rather than pass a void *, make this take a hdmi_any_infoframe *
and require callers to explicitly cast.

This is an internal API and therefore less likely to be abused, so again
rather bikesheddy.

 +static const char *hdmi_nups_get_name(enum hdmi_nups nups)
 +{
 + switch (nups) {
 + case HDMI_NUPS_UNKNOWN:
 + return No Known Non-uniform Scaling;

s/No Known/Unknown/?

 +static void hdmi_avi_infoframe_log(const char *level,
 +struct device *dev,
 +struct hdmi_avi_infoframe *frame)
 +{
 + hdmi_infoframe_log_header(level, dev, frame);
 +
 + hdmi_log(colorspace: %s\n,
 + hdmi_colorspace_get_name(frame-colorspace));
 + hdmi_log(scan mode: %s\n,
 + hdmi_scan_mode_get_name(frame-scan_mode));
 + hdmi_log(colorimetry: %s\n,
 + hdmi_colorimetry_get_name(frame-colorimetry));
 + hdmi_log(picture aspect: %s\n,
 + hdmi_picture_aspect_get_name(frame-picture_aspect));
 + hdmi_log(active aspect: %s\n,
 + hdmi_active_aspect_get_name(frame-active_aspect));
 + hdmi_log(itc: %s\n, frame-itc ? IT Content : No Data);
 + hdmi_log(extended colorimetry: %s\n,
 + 
 hdmi_extended_colorimetry_get_name(frame-extended_colorimetry));
 + hdmi_log(quantization range: %s\n,
 + 
 hdmi_quantization_range_get_name(frame-quantization_range));
 + hdmi_log(nups: %s\n, hdmi_nups_get_name(frame-nups));
 + hdmi_log(video code: %d\n, frame-video_code);

This could be %u.

 + hdmi_log(ycc quantization range: %s\n,
 + 
 hdmi_ycc_quantization_range_get_name(frame-ycc_quantization_range));
 + hdmi_log(hdmi content type: %s\n,
 + hdmi_content_type_get_name(frame-content_type));
 + hdmi_log(pixel repeat: %d\n, frame-pixel_repeat);
 + hdmi_log(bar top %d, bottom %d, left %d, right %d\n,
 + frame-top_bar, frame-bottom_bar,
 + frame-left_bar, frame-right_bar);

Same here.

 +static const char *
 +hdmi_audio_coding_type_get_name(enum hdmi_audio_coding_type coding_type)
 +{
 + switch (coding_type) {
 + case HDMI_AUDIO_CODING_TYPE_STREAM:
 + return Refer to Stream Header;
[...]
 + case HDMI_AUDIO_CODING_TYPE_CXT:
 + return Refer to CXT;

These aren't really names, but I can't come up with anything better.

 +static const char *
 +hdmi_audio_coding_type_ext_get_name(enum hdmi_audio_coding_type_ext ctx)
 +{
 +  

Re: [PATCHv2 0/3] hdmi: add unpack and logging functions

[Thierry Reding]

On Thu, Dec 11, 2014 at 09:57:54AM +0100, Hans Verkuil wrote:
 Hi Thierry,
 
 On 12/02/14 13:08, Hans Verkuil wrote:
  This patch series adds new HDMI 2.0/CEA-861-F defines to hdmi.h and
  adds unpacking and logging functions to hdmi.c. It also uses those
  in the V4L2 adv7842 driver (and they will be used in other HDMI drivers
  once this functionality is merged).
  
  Patches 2 and 3 have been posted before by Martin Bugge. It stalled, but
  I am taking over from Martin to try and get this is. I want to use this
  in a bunch of v4l2 drivers, so I would really like to see this merged.
  
  Changes since v1:
  
  - rename HDMI_CONTENT_TYPE_NONE to HDMI_CONTENT_TYPE_GRAPHICS to conform
to CEA-861-F.
  - added missing HDMI_AUDIO_CODING_TYPE_CXT.
  - Be explicit: out of range values are called Invalid, reserved
values are called Reserved.
  - Incorporated most of Thierry's suggestions. Exception: I didn't
create ..._get_name(buffer, length, ...) functions. I think it makes
the API awkward and I am not convinced that it is that useful.
I also kept No Data since that's what CEA-861-F calls it. I also
think that No Data is a better description than None since it
really means that nobody bothered to fill this in.
  
  Please let me know if there are more things that need to be addressed in
  these patches before they can be merged.
 
 Any comments about this v2?

Sorry for taking so long. This got burried under a lot of other stuff. I
have some minor comments to patch 2/3, but on the whole this looks very
nice.

 If not, is this something you or someone else from dri-devel will
 take, or can it be merged through the media git repository?

I'm not aware of anyone currently doing work on this for DRM, so I think
it'd be fine if you took it through the media git tree, especially since
patch 3/3 clearly belongs there.

If we ever need to resolve dependencies between this and new work in DRM
we could set up a stable branch containing patches 1/3 and 2/3 which can
be merged into both trees.

Thierry


pgpm9NwSlP37s.pgp
Description: PGP signature

Re: [PATCH 1/3] hdmi: add new HDMI 2.0 defines

[Thierry Reding]

On Fri, Nov 28, 2014 at 03:50:49PM +0100, Hans Verkuil wrote:
 From: Hans Verkuil hans.verk...@cisco.com
 
 Add new Video InfoFrame colorspace information introduced in HDMI 2.0
 and new Audio Coding Extension Types, also from HDMI 2.0.
 
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 ---
  include/linux/hdmi.h | 20 
  1 file changed, 20 insertions(+)
 
 diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
 index 11c0182..38fd2a0 100644
 --- a/include/linux/hdmi.h
 +++ b/include/linux/hdmi.h
 @@ -37,6 +37,8 @@ enum hdmi_colorspace {
   HDMI_COLORSPACE_RGB,
   HDMI_COLORSPACE_YUV422,
   HDMI_COLORSPACE_YUV444,
 + HDMI_COLORSPACE_YUV420,
 + HDMI_COLORSPACE_IDO_DEFINED = 7,
  };
  
  enum hdmi_scan_mode {
 @@ -77,6 +79,10 @@ enum hdmi_extended_colorimetry {
   HDMI_EXTENDED_COLORIMETRY_S_YCC_601,
   HDMI_EXTENDED_COLORIMETRY_ADOBE_YCC_601,
   HDMI_EXTENDED_COLORIMETRY_ADOBE_RGB,
 +
 + /* The following EC values are only defined in CEA-861-F. */
 + HDMI_EXTENDED_COLORIMETRY_BT2020_CONST_LUM,
 + HDMI_EXTENDED_COLORIMETRY_BT2020,
  };
  
  enum hdmi_quantization_range {
 @@ -201,9 +207,23 @@ enum hdmi_audio_sample_frequency {
  
  enum hdmi_audio_coding_type_ext {
   HDMI_AUDIO_CODING_TYPE_EXT_STREAM,
 +
 + /*
 +  * The next three CXT values are defined in CEA-861-E only.
 +  * They do not exist in older versions, and in CEA-861-F they are
 +  * defined as 'Not in use'.
 +  */
   HDMI_AUDIO_CODING_TYPE_EXT_HE_AAC,
   HDMI_AUDIO_CODING_TYPE_EXT_HE_AAC_V2,
   HDMI_AUDIO_CODING_TYPE_EXT_MPEG_SURROUND,
 +
 + /* The following CXT values are only defined in CEA-861-F. */
 + HDMI_AUDIO_CODING_TYPE_EXT_MPEG4_HE_AAC,
 + HDMI_AUDIO_CODING_TYPE_EXT_MPEG4_HE_AAC_V2,
 + HDMI_AUDIO_CODING_TYPE_EXT_MPEG4_AAC_LC,
 + HDMI_AUDIO_CODING_TYPE_EXT_DRA,
 + HDMI_AUDIO_CODING_TYPE_EXT_MPEG_HE_AAC_SURROUND,
 + HDMI_AUDIO_CODING_TYPE_EXT_MPEG_AAC_LC_SURROUND = 10,

I think the last two should be MPEG4_{HE_AAC,AAC}_SURROUND, and with
that fixed:

Reviewed-by: Thierry Reding tred...@nvidia.com


pgp8Xl95hNsoz.pgp
Description: PGP signature

Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames

[Thierry Reding]

On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
 From: Martin Bugge marbu...@cisco.com
 
 When receiving video it is very useful to be able to unpack the InfoFrames.
 Logging is useful as well, both for transmitters and receivers.
 
 Especially when implementing the VIDIOC_LOG_STATUS ioctl (supported by many
 V4L2 drivers) for a receiver it is important to be able to easily log what
 the InfoFrame contains. This greatly simplifies debugging.
 
 Signed-off-by: Martin Bugge marbu...@cisco.com
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 ---
  drivers/video/hdmi.c | 622 
 ++-
  include/linux/hdmi.h |   3 +
  2 files changed, 618 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
 index 9e758a8..9f0f554 100644
 --- a/drivers/video/hdmi.c
 +++ b/drivers/video/hdmi.c
 @@ -27,10 +27,10 @@
  #include linux/export.h
  #include linux/hdmi.h
  #include linux/string.h
 +#include linux/device.h
  
 -static void hdmi_infoframe_checksum(void *buffer, size_t size)
 +static u8 hdmi_infoframe_calc_checksum(u8 *ptr, size_t size)

I'd personally keep the name here.

 @@ -434,3 +441,604 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, void 
 *buffer, size_t size)
   return length;
  }
  EXPORT_SYMBOL(hdmi_infoframe_pack);
 +
 +static const char *hdmi_infoframe_type_txt(enum hdmi_infoframe_type type)

Perhaps: hdmi_infoframe_type_get_name()?

 +{
 + switch (type) {
 + case HDMI_INFOFRAME_TYPE_VENDOR: return Vendor;
 + case HDMI_INFOFRAME_TYPE_AVI: return Auxiliary Video Information 
 (AVI);
 + case HDMI_INFOFRAME_TYPE_SPD: return Source Product Description (SPD);
 + case HDMI_INFOFRAME_TYPE_AUDIO: return Audio;

I'd prefer case ...: and return ...; on separate lines for
readability.

 + }
 + return Invalid/Unknown;
 +}

Maybe include the numerical value here? Of course that either means that
callers must pass in a buffer or we sacrifice thread-safety. The buffer
could be optional, somewhat like this:

const char *hdmi_infoframe_get_name(char *buffer, size_t length,
enum hdmi_infoframe_type type)
{
const char *name = NULL;

switch (type) {
case HDMI_INFOFRAME_TYPE_VENDOR:
name = Vendor;
break;
...
}

if (buffer) {
if (!name)
snprintf(buffer, length, unknown (%d), type);
else
snprintf(buffer, length, name);

name = buffer;
}

return name;
}

That way the function would be generally useful and could even be made
publicly available.

 +static void hdmi_infoframe_log_header(struct device *dev, void *f)
 +{
 + struct hdmi_any_infoframe *frame = f;
 + dev_info(dev, HDMI infoframe: %s, version %d, length %d\n,
 + hdmi_infoframe_type_txt(frame-type), frame-version, 
 frame-length);
 +}
 +
 +static const char *hdmi_colorspace_txt(enum hdmi_colorspace colorspace)
 +{
 + switch (colorspace) {
 + case HDMI_COLORSPACE_RGB: return RGB;
 + case HDMI_COLORSPACE_YUV422: return YCbCr 4:2:2;
 + case HDMI_COLORSPACE_YUV444: return YCbCr 4:4:4;
 + case HDMI_COLORSPACE_YUV420: return YCbCr 4:2:0;
 + case HDMI_COLORSPACE_IDO_DEFINED: return IDO Defined;
 + }
 + return Future;
 +}

Similar comments as for the above.

 +static const char *hdmi_scan_mode_txt(enum hdmi_scan_mode scan_mode)
 +{
 + switch(scan_mode) {
 + case HDMI_SCAN_MODE_NONE: return No Data;
 + case HDMI_SCAN_MODE_OVERSCAN: return Composed for overscanned display;
 + case HDMI_SCAN_MODE_UNDERSCAN: return Composed for underscanned 
 display;
 + }
 + return Future;
 +}

This isn't really a name any more, I think it should either stick to
names like None, Overscan, Underscan or it should return a
description, in which case hdmi_scan_mode_get_description() might be
more accurate for a name.

 +static const char *hdmi_colorimetry_txt(enum hdmi_colorimetry colorimetry)
 +{
 + switch(colorimetry) {
 + case HDMI_COLORIMETRY_NONE: return No Data;
 + case HDMI_COLORIMETRY_ITU_601: return ITU601;
 + case HDMI_COLORIMETRY_ITU_709: return ITU709;
 + case HDMI_COLORIMETRY_EXTENDED: return Extended;
 + }
 + return Invalid/Unknown;
 +}

These are names again, so same comments as for the infoframe type. And
perhaps No Data - None in that case.

 +
 +static const char *hdmi_picture_aspect_txt(enum hdmi_picture_aspect 
 picture_aspect)
 +{
 + switch (picture_aspect) {
 + case HDMI_PICTURE_ASPECT_NONE: return No Data;
 + case HDMI_PICTURE_ASPECT_4_3: return 4:3;
 + case HDMI_PICTURE_ASPECT_16_9: return 16:9;
 + }
 + return Future;
 +}

Same here.

 +static const 

Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames

[Thierry Reding]

On Mon, Dec 01, 2014 at 02:48:47PM +0100, Hans Verkuil wrote:
 Hi Thierry,
 
 Thanks for the review, see my comments below.
 
 On 12/01/2014 02:15 PM, Thierry Reding wrote:
  On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
[...]
  +{
  +  switch (type) {
  +  case HDMI_INFOFRAME_TYPE_VENDOR: return Vendor;
  +  case HDMI_INFOFRAME_TYPE_AVI: return Auxiliary Video Information 
  (AVI);
  +  case HDMI_INFOFRAME_TYPE_SPD: return Source Product Description (SPD);
  +  case HDMI_INFOFRAME_TYPE_AUDIO: return Audio;
  
  I'd prefer case ...: and return ...; on separate lines for
  readability.
 
 I actually think that makes it *less* readable. If you really want that, then 
 I'll
 change it, but I would suggest that you try it yourself first to see if it is
 really more readable for you. It isn't for me, so I'll keep this for the next
 version.

I did, and I still think separate lines are more readable, especially if
you throw in a blank line after the return ...;. Anyway, I could keep
my OCD in check if it weren't for the fact that half of these are the
cause for checkpatch to complain. And then if you change the ones that
checkpatch wants you to change, all the others would be inconsistent and
then I'd complain about the inconsistency...

checkpatch flagged a couple other issues, please make sure to address
those as well.

  Maybe include the numerical value here? Of course that either means that
  callers must pass in a buffer or we sacrifice thread-safety. The buffer
  could be optional, somewhat like this:
  
  const char *hdmi_infoframe_get_name(char *buffer, size_t length,
  enum hdmi_infoframe_type type)
  {
  const char *name = NULL;
  
  switch (type) {
  case HDMI_INFOFRAME_TYPE_VENDOR:
  name = Vendor;
  break;
  ...
  }
  
  if (buffer) {
  if (!name)
  snprintf(buffer, length, unknown (%d), type);
  else
  snprintf(buffer, length, name);
  
  name = buffer;
  }
  
  return name;
  }
  
  That way the function would be generally useful and could even be made
  publicly available.
 
 I would do this only where it makes sense. Some of these fields have only one 
 or
 two reserved bits left, and in that case is it easier to just say something
 like Reserved (3) and do that for each reserved value.

Okay.

  +/**
  + * hdmi_infoframe_log() - log info of HDMI infoframe
  + * @dev: device
  + * @frame: HDMI infoframe
  + */
  +void hdmi_infoframe_log(struct device *dev, union hdmi_infoframe *frame)
  +{
  +  switch (frame-any.type) {
  +  case HDMI_INFOFRAME_TYPE_AVI:
  +  hdmi_avi_infoframe_log(dev, frame-avi);
  +  break;
  +  case HDMI_INFOFRAME_TYPE_SPD:
  +  hdmi_spd_infoframe_log(dev, frame-spd);
  +  break;
  +  case HDMI_INFOFRAME_TYPE_AUDIO:
  +  hdmi_audio_infoframe_log(dev, frame-audio);
  +  break;
  +  case HDMI_INFOFRAME_TYPE_VENDOR:
  +  hdmi_vendor_any_infoframe_log(dev, frame-vendor);
  +  break;
  +  default:
  +  WARN(1, Bad infoframe type %d\n, frame-any.type);
  
  Does it make sense for this to be WARN? It's perfectly legal for future
  devices to expose new types of infoframes. Perhaps even expected. But if
  we want to keep this here to help get bug reports so that we don't
  forget to update this code, then maybe we should do the same wherever we
  query the name of enum values above.
 
 I'll drop the WARN from the log function. I think it should also be dropped
 from the unpack. The only place it makes sense is for pack() since there the
 data comes from the driver, not from an external source.

Sounds good.

Thierry


pgpQTn1nRD50A.pgp
Description: PGP signature

[PATCH v2] [media] s5p-jpeg: Only build suspend/resume for PM

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

If power management is disabled these function become unused, so there
is no reason to build them. This fixes a couple of build warnings when
PM(_SLEEP,_RUNTIME) is not enabled.

Acked-by: Geert Uytterhoeven ge...@linux-m68k.org
Signed-off-by: Thierry Reding tred...@nvidia.com
---
Changes in v2:
- add #endif comment for readability
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index e525a7c8d885..ec7339e84b57 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2632,6 +2632,7 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
return 0;
 }
 
+#if defined(CONFIG_PM_RUNTIME) || defined(CONFIG_PM_SLEEP)
 static int s5p_jpeg_runtime_suspend(struct device *dev)
 {
struct s5p_jpeg *jpeg = dev_get_drvdata(dev);
@@ -2681,7 +2682,9 @@ static int s5p_jpeg_runtime_resume(struct device *dev)
 
return 0;
 }
+#endif /* CONFIG_PM_RUNTIME || CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM_SLEEP
 static int s5p_jpeg_suspend(struct device *dev)
 {
if (pm_runtime_suspended(dev))
@@ -2697,6 +2700,7 @@ static int s5p_jpeg_resume(struct device *dev)
 
return s5p_jpeg_runtime_resume(dev);
 }
+#endif
 
 static const struct dev_pm_ops s5p_jpeg_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(s5p_jpeg_suspend, s5p_jpeg_resume)
-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] [media] s5p-fimc: Only build suspend/resume for PM

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

If power management is disabled these functions become unused, so there
is no reason to build them. This fixes a couple of build warnings when
PM(_SLEEP,_RUNTIME) is not enabled.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
Changes in v2:
- add #endif comment for readability

 drivers/media/platform/exynos4-is/fimc-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index b70fd996d794..aee92d908e49 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -832,6 +832,7 @@ err:
return -ENXIO;
 }
 
+#if defined(CONFIG_PM_RUNTIME) || defined(CONFIG_PM_SLEEP)
 static int fimc_m2m_suspend(struct fimc_dev *fimc)
 {
unsigned long flags;
@@ -870,6 +871,7 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
 
return 0;
 }
+#endif /* CONFIG_PM_RUNTIME || CONFIG_PM_SLEEP */
 
 static const struct of_device_id fimc_of_match[];
 
-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] s5p-jpeg: Only build suspend/resume for PM

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

If power management is disabled these function become unused, so there
is no reason to build them. This fixes a couple of build warnings when
PM(_SLEEP,_RUNTIME) is not enabled.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index e525a7c8d885..cc5d6bd40256 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2632,6 +2632,7 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
return 0;
 }
 
+#if defined(CONFIG_PM_RUNTIME) || defined(CONFIG_PM_SLEEP)
 static int s5p_jpeg_runtime_suspend(struct device *dev)
 {
struct s5p_jpeg *jpeg = dev_get_drvdata(dev);
@@ -2681,7 +2682,9 @@ static int s5p_jpeg_runtime_resume(struct device *dev)
 
return 0;
 }
+#endif
 
+#ifdef CONFIG_PM_SLEEP
 static int s5p_jpeg_suspend(struct device *dev)
 {
if (pm_runtime_suspended(dev))
@@ -2697,6 +2700,7 @@ static int s5p_jpeg_resume(struct device *dev)
 
return s5p_jpeg_runtime_resume(dev);
 }
+#endif
 
 static const struct dev_pm_ops s5p_jpeg_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(s5p_jpeg_suspend, s5p_jpeg_resume)
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] s5p-fimc: Only build suspend/resume for PM

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

If power management is disabled these functions become unused, so there
is no reason to build them. This fixes a couple of build warnings when
PM(_SLEEP,_RUNTIME) is not enabled.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index b70fd996d794..8e7435bfa1f9 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -832,6 +832,7 @@ err:
return -ENXIO;
 }
 
+#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
 static int fimc_m2m_suspend(struct fimc_dev *fimc)
 {
unsigned long flags;
@@ -870,6 +871,7 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
 
return 0;
 }
+#endif
 
 static const struct of_device_id fimc_of_match[];
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] s5p-fimc: Only build suspend/resume for PM

[Thierry Reding]

On Thu, Oct 02, 2014 at 10:43:21AM -0300, Mauro Carvalho Chehab wrote:
 Em Thu, 02 Oct 2014 10:48:11 +0200
 Thierry Reding thierry.red...@gmail.com escreveu:
 
  From: Thierry Reding tred...@nvidia.com
  
  If power management is disabled these functions become unused, so there
  is no reason to build them. This fixes a couple of build warnings when
  PM(_SLEEP,_RUNTIME) is not enabled.
  
  Signed-off-by: Thierry Reding tred...@nvidia.com
  ---
   drivers/media/platform/exynos4-is/fimc-core.c | 2 ++
   1 file changed, 2 insertions(+)
  
  diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
  b/drivers/media/platform/exynos4-is/fimc-core.c
  index b70fd996d794..8e7435bfa1f9 100644
  --- a/drivers/media/platform/exynos4-is/fimc-core.c
  +++ b/drivers/media/platform/exynos4-is/fimc-core.c
  @@ -832,6 +832,7 @@ err:
  return -ENXIO;
   }
   
  +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
   static int fimc_m2m_suspend(struct fimc_dev *fimc)
   {
  unsigned long flags;
  @@ -870,6 +871,7 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
   
  return 0;
   }
  +#endif
 
 The patch obviously is correct, but I'm wandering if aren't there a way
 to avoid the if/endif.
 
 Not tested here, but perhaps if we mark those functions as inline, the
 C compiler would do the right thing without generating any warnings.
 
 If not, maybe we could use some macro like:
 
 #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
   #define PM_SLEEP_FUNC
 #else
   #define PM_SLEEP_FUNC   inline
 #endif
 
 And put it at pm.h.
 
 That should be enough to shut up to warning without adding any footprint
 if PM is disabled.

I think you could use __maybe_unused to that effect, but it has the
disadvantage of hiding such messages forever. For instance if the
suspend/resume code was ever to be removed, then you wouldn't get a
warning at all.

And there's a corresponding #ifdef for the fimc_runtime_{suspend,resume}
and fimc_{suspend,resume} already anyway, so I don't see much point in
trying to avoid this particular #ifdef.

Thierry


pgpbBR6pTAJeD.pgp
Description: PGP signature

Re: [PATCH v2 1/5] video: move mediabus format definition to a more standard place

[Thierry Reding]

On Tue, Sep 30, 2014 at 09:37:57AM +0200, Boris Brezillon wrote:
 On Mon, 29 Sep 2014 23:41:09 +0300
 Laurent Pinchart laurent.pinch...@ideasonboard.com wrote:
[...]
  Incidentally, patch 2/5 in this series is missing a documentation update ;-)
 
 Yep, regarding this patch, I wonder if it's really necessary to add
 new formats to the v4l2_mbus_pixelcode enum.
 If we want to move to this new common definition (across the video
 related subsytems), we should deprecate the old enum
 v4l2_mbus_pixelcode, and this start by not adding new formats, don't
 you think ?

I agree in general, but I think it could prove problematic in practice.
If somebody wants to use one of the new codes but is using the V4L2 enum
they have a problem.

That said, given that there is now a unified enum people will hopefully
start converting drivers to it instead.

Thierry


pgp3vJhp6O6EZ.pgp
Description: PGP signature

Re: [PATCH v2 1/5] video: move mediabus format definition to a more standard place

[Thierry Reding]

On Mon, Sep 29, 2014 at 04:02:39PM +0200, Boris Brezillon wrote:
 Rename mediabus formats and move the enum into a separate header file so
 that it can be used by DRM/KMS subsystem without any reference to the V4L2
 subsystem.
 
 Old V4L2_MBUS_FMT_ definitions are now macros that points to VIDEO_BUS_FMT_
 definitions.
 
 Signed-off-by: Boris BREZILLON boris.brezil...@free-electrons.com
 Acked-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
 ---
  include/uapi/linux/Kbuild |   1 +
  include/uapi/linux/v4l2-mediabus.h| 183 
 +++---
  include/uapi/linux/video-bus-format.h | 127 +++
  3 files changed, 207 insertions(+), 104 deletions(-)
  create mode 100644 include/uapi/linux/video-bus-format.h

Hi Mauro,

Do you have any objections to me merging patches 1 and 2 through the
drm/panel tree given the dependency of the later patches in the series
on these new constants? If you want I can provide a stable branch once
v3.18-rc1 is out for you to pull into you tree to resolve possible
conflicts.

Thierry


pgp80jlBo2KxU.pgp
Description: PGP signature

Re: [PATCH 3/5] drm: add bus_formats and nbus_formats fields to drm_display_info

[Thierry Reding]

On Tue, Jul 22, 2014 at 02:23:45PM +0200, Boris BREZILLON wrote:
 Add bus_formats and nbus_formats fields and
 drm_display_info_set_bus_formats helper function to specify the bus
 formats supported by a given display.
 
 This information can be used by display controller drivers to configure
 the output interface appropriately (i.e. RGB565, RGB666 or RGB888 on raw
 RGB or LVDS busses).
 
 Signed-off-by: Boris BREZILLON boris.brezil...@free-electrons.com
 ---
  drivers/gpu/drm/drm_crtc.c | 28 
  include/drm/drm_crtc.h |  8 
  2 files changed, 36 insertions(+)
 
 diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
 index c808a09..50c8395 100644
 --- a/drivers/gpu/drm/drm_crtc.c
 +++ b/drivers/gpu/drm/drm_crtc.c
 @@ -825,6 +825,34 @@ static void drm_mode_remove(struct drm_connector 
 *connector,
   drm_mode_destroy(connector-dev, mode);
  }
  
 +/*
 + * drm_display_info_set_bus_formats - set the supported bus formats
 + * @info: display info to store bus formats in
 + * @fmts: array containing the supported bus formats
 + * @nfmts: the number of entries in the fmts array
 + *
 + * Store the suppported bus formats in display info structure.
 + */
 +int drm_display_info_set_bus_formats(struct drm_display_info *info,
 +  const enum video_bus_format *fmts,
 +  int nfmts)

Can you make nfmts unsigned please?

 +{
 + enum video_bus_format *formats = NULL;
 +
 + if (fmts  nfmts) {
 + formats = kmemdup(fmts, sizeof(*fmts) * nfmts, GFP_KERNEL);
 + if (!formats)
 + return -ENOMEM;
 + }
 +
 + kfree(info-bus_formats);
 + info-bus_formats = formats;
 + info-nbus_formats = formats ? nfmts : 0;

And perhaps check for formats == NULL  nfmts != 0 since that's not a
valid pair of values. Then you can simply assign this directly without
relying on the value of formats.

Also other variable names use num_ as a prefix instead of n, so if
you're going to respin anyway might as well make the names more
consistent.

 +
 + return 0;
 +}
 +EXPORT_SYMBOL(drm_display_info_set_bus_formats);
 +
  /**
   * drm_connector_init - Init a preallocated connector
   * @dev: DRM device
 diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
 index e529b68..957729b 100644
 --- a/include/drm/drm_crtc.h
 +++ b/include/drm/drm_crtc.h
 @@ -31,6 +31,7 @@
  #include linux/idr.h
  #include linux/fb.h
  #include linux/hdmi.h
 +#include linux/video-bus-format.h
  #include drm/drm_mode.h
  #include drm/drm_fourcc.h
  #include drm/drm_modeset_lock.h
 @@ -121,6 +122,9 @@ struct drm_display_info {
   enum subpixel_order subpixel_order;
   u32 color_formats;
  
 + const enum video_bus_format *bus_formats;
 + int nbus_formats;

unsigned int here too, please.

Thierry


pgpsFlpGhOy0x.pgp
Description: PGP signature

Re: [PATCH 5/5] drm: panel: simple-panel: add bus format information for foxlink panel

[Thierry Reding]

On Tue, Jul 22, 2014 at 02:23:47PM +0200, Boris BREZILLON wrote:
 Foxlink's fl500wvr00-a0t supports RGB888 format.
 
 Signed-off-by: Boris BREZILLON boris.brezil...@free-electrons.com
 ---
  drivers/gpu/drm/panel/panel-simple.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/gpu/drm/panel/panel-simple.c 
 b/drivers/gpu/drm/panel/panel-simple.c
 index 42fd6d1..f1e49fd 100644
 --- a/drivers/gpu/drm/panel/panel-simple.c
 +++ b/drivers/gpu/drm/panel/panel-simple.c
 @@ -428,6 +428,7 @@ static const struct panel_desc foxlink_fl500wvr00_a0t = {
   .width = 108,
   .height = 65,
   },
 + .bus_format = VIDEO_BUS_FMT_RGB888_1X24,

This is really equivalent to .bpc = 8. Didn't you say you had other
use-cases where .bpc wasn't sufficient?

Thierry


pgpACzaTCYnMu.pgp
Description: PGP signature

[PATCH] dma-buf/fence: Fix a kerneldoc warning

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

kerneldoc doesn't know how to parse variables, so don't let it try.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/dma-buf/fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 4222cb2aa96a..7bb9d65d9a2c 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -29,7 +29,7 @@
 EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
 EXPORT_TRACEPOINT_SYMBOL(fence_emit);
 
-/**
+/*
  * fence context counter: each execution context should have its own
  * fence context, this allows checking if fences belong to the same
  * context or not. One device can have multiple separate contexts,
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] dma-buf/fence: Fix one more kerneldoc warning

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

The seqno_fence_init() function's cond argument isn't described in the
kerneldoc comment. Fix that to silence a warning when building DocBook
documentation.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 include/linux/seqno-fence.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/seqno-fence.h b/include/linux/seqno-fence.h
index 3d6003de4b0d..a1ba6a5ccdd6 100644
--- a/include/linux/seqno-fence.h
+++ b/include/linux/seqno-fence.h
@@ -62,6 +62,7 @@ to_seqno_fence(struct fence *fence)
  * @context: the execution context this fence is a part of
  * @seqno_ofs: the offset within @sync_buf
  * @seqno: the sequence # to signal on
+ * @cond: fence wait condition
  * @ops: the fence_ops for operations on this seqno fence
  *
  * This function initializes a struct seqno_fence with passed parameters,
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.

[Thierry Reding]

On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote:
[...]
 diff --git 
 a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt 
 b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt
[...]
 @@ -0,0 +1,7 @@
 +Eukrea DVI-SVGA (800x600 pixels) DVI output.
[...]
 diff --git 
 a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt 
 b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt
[...]
 @@ -0,0 +1,7 @@
 +Eukrea DVI-VGA (640x480 pixels) DVI output.

DVI outputs shouldn't be using the panel framework and this binding at
all. DVI usually has the means to determine all of this by itself. Why
do you need to represent this as a panel in device tree?

Thierry


pgpuEcZ4BBFxh.pgp
Description: PGP signature

Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.

[Thierry Reding]

On Tue, Jun 24, 2014 at 02:52:11PM -0500, Rob Herring wrote:
 On Tue, Jun 24, 2014 at 10:06 AM, Russell King - ARM Linux 
 li...@arm.linux.org.uk wrote:
[...]
  On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote:
[...]
  diff --git 
  a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt 
  b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt
[...]
  @@ -0,0 +1,7 @@
  +Eukrea DVI-VGA (640x480 pixels) DVI output.
  +
  +Required properties:
  +- compatible: should be eukrea,mbimxsd51-dvi-vga
  +
  +This binding is compatible with the simple-panel binding, which is 
  specified
  +in simple-panel.txt in this directory.
 
 Seems like we could just have a list of compatible strings rather than
 a mostly duplicated file.

We've been doing it this way for all other panels.

  diff --git a/drivers/gpu/drm/panel/panel-simple.c 
  b/drivers/gpu/drm/panel/panel-simple.c
  index a251361..adc40a7 100644
  --- a/drivers/gpu/drm/panel/panel-simple.c
  +++ b/drivers/gpu/drm/panel/panel-simple.c
  @@ -403,6 +403,80 @@ static const struct panel_desc edt_etm0700g0dh6 = {
},
   };
 
  +static const struct drm_display_mode eukrea_mbimxsd51_cmoqvga_mode = {
  + .clock = 6500,
  + .hdisplay = 320,
  + .hsync_start = 320 + 38,
  + .hsync_end = 320 + 38 + 20,
  + .htotal = 320 + 38 + 20 + 30,
  + .vdisplay = 240,
  + .vsync_start = 240 + 15,
  + .vsync_end = 240 + 15 + 4,
  + .vtotal = 240 + 15 + 4 + 3,
  + .vrefresh = 60,
  + .pol_flags = DRM_MODE_FLAG_POL_PIXDATA_NEGEDGE |
  +  DRM_MODE_FLAG_POL_DE_LOW,
 
 Why aren't you using:
 
 Documentation/devicetree/bindings/video/display-timing.txt

Because it's redundant information. We need to have a compatible for the
panel in the device tree anyway and that already implicitly defines the
display mode.

Thierry


pgprSon35YW3J.pgp
Description: PGP signature

Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.

[Thierry Reding]

On Tue, Jun 24, 2014 at 11:56:39PM +0200, Eric Bénard wrote:
 Hi Thierry,
 
 Le Tue, 24 Jun 2014 23:49:37 +0200,
 Thierry Reding thierry.red...@gmail.com a écrit :
 
  On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote:
  [...]
   diff --git 
   a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt 
   b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt
  [...]
   @@ -0,0 +1,7 @@
   +Eukrea DVI-SVGA (800x600 pixels) DVI output.
  [...]
   diff --git 
   a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt 
   b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt
  [...]
   @@ -0,0 +1,7 @@
   +Eukrea DVI-VGA (640x480 pixels) DVI output.
  
  DVI outputs shouldn't be using the panel framework and this binding at
  all. DVI usually has the means to determine all of this by itself. Why
  do you need to represent this as a panel in device tree?
  
 because on this very simple display board, we only have DVI LVDS signals
 without the I2C to detect the display.

That's unfortunate. In that case perhaps a better approach would be to
add a video timings node to the device that provides the DVI output?

The panel bindings are really for internal panels and should define all
of their properties. That's also why they need a specific compatible
string.

What the above two bindings define are really connectors with a fixed
resolution rather than panels.

Thierry


pgpB2GyghDLMb.pgp
Description: PGP signature

Re: [PATCH v14 06/10] drm: drm_display_mode: add signal polarity flags

[Thierry Reding]

On Tue, Jun 24, 2014 at 03:57:46PM +0100, Russell King - ARM Linux wrote:
 On Mon, Jun 16, 2014 at 12:11:20PM +0200, Denis Carikli wrote:
  We need a way to pass signal polarity informations
between DRM panels, and the display drivers.
  
  To do that, a pol_flags field was added to drm_display_mode.
  
  Signed-off-by: Denis Carikli de...@eukrea.com
 
 This patch needs an ack from the DRM people - can someone review it
 please?  This series has now been round 14 revisions and it's about
 time it was properly reviewed - or a statement made if it's
 unacceptable.

I didn't follow all of the earlier discussions around this, but it seems
to me like data-enable polarity and the pixel data edge flags are
properties of the interface rather than the video mode.

struct drm_display_mode represents the video timings and I'm not sure if
it's a good idea to extend it with this type of information.

Maybe we need to add a separate type of device to store these parameters
(much like we've done for MIPI DSI devices).

Thierry


pgpS04DEW9Xje.pgp
Description: PGP signature

Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5

[Thierry Reding]

On Thu, Jun 19, 2014 at 02:28:14PM +0200, Daniel Vetter wrote:
 On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
   With these changes, can we pull the android sync logic out of
   drivers/staging/ now?
 
  Afaik the google guys never really looked at this and acked it. So I'm not
  sure whether they'll follow along. The other issue I have as the
  maintainer of gfx driver is that I don't want to implement support for two
  different sync object primitives (once for dma-buf and once for android
  syncpts), and my impression thus far has been that even with this we're
  not there.
 
  I'm trying to get our own android guys to upstream their i915 syncpts
  support, but thus far I haven't managed to convince them to throw people's
  time at this.
 
  This has been discussed a fair bit internally recently and some of our
  GPU experts have raised concerns that this may result in seriously
  degraded performance in our proprietary graphics stack. Now I don't care
  very much for the proprietary graphics stack, but by extension I would
  assume that the same restrictions are relevant for any open-source
  driver as well.
 
  I'm still trying to fully understand all the implications and at the
  same time get some of the people who raised concerns to join in this
  discussion. As I understand it the concern is mostly about explicit vs.
  implicit synchronization and having this mechanism in the kernel will
  implicitly synchronize all accesses to these buffers even in cases where
  it's not needed (read vs. write locks, etc.). In one particular instance
  it was even mentioned that this kind of implicit synchronization can
  lead to deadlocks in some use-cases (this was mentioned for Android
  compositing, but I suspect that the same may happen for Wayland or X
  compositors).
 
 Well the implicit fences here actually can't deadlock. That's the
 entire point behind using ww mutexes. I've also heard tons of
 complaints about implicit enforced syncing (especially from opencl
 people), but in the end drivers and always expose unsynchronized
 access for specific cases. We do that in i915 for upload buffers and
 other fun stuff. This is about shared stuff across different drivers
 and different processes.

Tegra K1 needs to share buffers across different drivers even for very
basic use-cases since the GPU and display drivers are separate. So while
I agree that the GPU driver can still use explicit synchronization for
internal work, things aren't that simple in general.

Let me try to reconstruct the use-case that caused the lock on Android:
the compositor uses a hardware overlay to display an image. The system
detects that there's little activity and instructs the compositor to put
everything into one image and scan out only that (for power efficiency).
Now with implicit locking the display driver has a lock on the image, so
the GPU (used for compositing) needs to wait for it before it can
composite everything into one image. But the display driver cannot
release the lock on the image until the final image has been composited
and can be displayed instead.

This may not be technically a deadlock, but it's still a stalemate.
Unless I'm missing something fundamental about DMA fences and ww mutexes
I don't see how you can get out of this situation.

Explicit vs. implicit synchronization may also become more of an issue
as buffers are imported from other sources (such as cameras).

 Finally I've never seen anyone from google or any android product guy
 push a real driver enabling for syncpts to upstream, and google itself
 has a bit a history of constantly exchanging their gfx framework for
 the next best thing. So I really doubt this is worthwhile to pursue in
 upstream with our essentially eternal api guarantees. At least until
 we see serious uptake from vendors and gfx driver guys. Unfortunately
 the Intel android folks are no exception here and haven't pushed
 anything like this in my direction yet at all. Despite multiple pokes
 from my side.
 
 So from my side I think we should move ahead with Maarten's work and
 figure the android side out once there's real interest.

The downside of that is that we may end up with two different ways to
synchronize buffers if it turns out that we can't make Android (or other
use-cases) work with DMA fence. However I don't think that justifies
postponing this patch set indefinitely.

Thierry


pgp0OB8pDlJzP.pgp
Description: PGP signature

Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5

[Thierry Reding]

On Thu, Jun 19, 2014 at 08:37:27AM +0200, Daniel Vetter wrote:
 On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote:
  On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote:
   Just to show it's easy.
   
   Android syncpoints can be mapped to a timeline. This removes the need
   to maintain a separate api for synchronization. I've left the android
   trace events in place, but the core fence events should already be
   sufficient for debugging.
   
   v2:
   - Call fence_remove_callback in sync_fence_free if not all fences have 
   fired.
   v3:
   - Merge Colin Cross' bugfixes, and the android fence merge optimization.
   v4:
   - Merge with the upstream fixes.
   v5:
   - Fix small style issues pointed out by Thomas Hellstrom.
   
   Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
   Acked-by: John Stultz john.stu...@linaro.org
   ---
drivers/staging/android/Kconfig  |1 
drivers/staging/android/Makefile |2 
drivers/staging/android/sw_sync.c|6 
drivers/staging/android/sync.c   |  913 
   +++---
drivers/staging/android/sync.h   |   79 ++-
drivers/staging/android/sync_debug.c |  247 +
drivers/staging/android/trace/sync.h |   12 
7 files changed, 609 insertions(+), 651 deletions(-)
create mode 100644 drivers/staging/android/sync_debug.c
  
  With these changes, can we pull the android sync logic out of
  drivers/staging/ now?
 
 Afaik the google guys never really looked at this and acked it. So I'm not
 sure whether they'll follow along. The other issue I have as the
 maintainer of gfx driver is that I don't want to implement support for two
 different sync object primitives (once for dma-buf and once for android
 syncpts), and my impression thus far has been that even with this we're
 not there.
 
 I'm trying to get our own android guys to upstream their i915 syncpts
 support, but thus far I haven't managed to convince them to throw people's
 time at this.

This has been discussed a fair bit internally recently and some of our
GPU experts have raised concerns that this may result in seriously
degraded performance in our proprietary graphics stack. Now I don't care
very much for the proprietary graphics stack, but by extension I would
assume that the same restrictions are relevant for any open-source
driver as well.

I'm still trying to fully understand all the implications and at the
same time get some of the people who raised concerns to join in this
discussion. As I understand it the concern is mostly about explicit vs.
implicit synchronization and having this mechanism in the kernel will
implicitly synchronize all accesses to these buffers even in cases where
it's not needed (read vs. write locks, etc.). In one particular instance
it was even mentioned that this kind of implicit synchronization can
lead to deadlocks in some use-cases (this was mentioned for Android
compositing, but I suspect that the same may happen for Wayland or X
compositors).

Thierry


pgpJIsKfAjjEU.pgp
Description: PGP signature

Re: [Intel-gfx] [RFC] set up an sync channel between audio and display driver (i.e. ALSA and DRM)

[Thierry Reding]

On Tue, May 20, 2014 at 12:04:38PM +0200, Daniel Vetter wrote:
 Also adding dri-devel and linux-media. Please don't forget these lists for
 the next round.
 -Daniel
 
 On Tue, May 20, 2014 at 12:02:04PM +0200, Daniel Vetter wrote:
  Adding Greg just as an fyi since we've chatted briefly about the avsink
  bus. Comments below.
  -Daniel
  
  On Tue, May 20, 2014 at 02:52:19AM +, Lin, Mengdong wrote:
   This RFC is based on previous discussion to set up a generic 
   communication channel between display and audio driver and
   an internal design of Intel MCG/VPG HDMI audio driver. It's still an 
   initial draft and your advice would be appreciated
   to improve the design.
   
   The basic idea is to create a new avsink module and let both drm and alsa 
   depend on it.
   This new module provides a framework and APIs for synchronization between 
   the display and audio driver.
   
   1. Display/Audio Client
   
   The avsink core provides APIs to create, register and lookup a 
   display/audio client.
   A specific display driver (eg. i915) or audio driver (eg. HD-Audio 
   driver) can create a client, add some resources
   objects (shared power wells, display outputs, and audio inputs, register 
   ops) to the client, and then register this
   client to avisink core. The peer driver can look up a registered client 
   by a name or type, or both. If a client gives
   a valid peer client name on registration, avsink core will bind the two 
   clients as peer for each other. And we
   expect a display client and an audio client to be peers for each other in 
   a system.
   
   int avsink_new_client ( const char *name,
   int type,   /* client type, display or audio 
   */
   struct module *module,
   void *context,
   const char *peer_name,
   struct avsink_client **client_ret);
   
   int avsink_free_client (struct avsink_client *client);
  
  
  Hm, my idea was to create a new avsink bus and let vga drivers register
  devices on that thing and audio drivers register as drivers. There's a bit
  more work involved in creating a full-blown bus, but it has a lot of
  upsides:
  - Established infrastructure for matching drivers (i.e. audio drivers)
against devices (i.e. avsinks exported by gfx drivers).
  - Module refcounting.
  - power domain handling and well-integrated into runtime pm.
  - Allows integration into componentized device framework since we're
dealing with a real struct device.
  - Better decoupling between gfx and audio side since registration is done
at runtime.
  - We can attach drv private date which the audio driver needs.

I think this would be another case where the interface framework[0]
could potentially be used. It doesn't give you all of the above, but
there's no reason it couldn't be extended. Then again, adding too much
would end up duplicating more of the driver core, so if something really
heavy-weight is required here, then the interface framework is not the
best option.

[0]: https://lkml.org/lkml/2014/5/13/525

   On system boots, the avsink module is loaded before the display and audio 
   driver module. And the display and audio
   driver may be loaded on parallel.
   * If a specific display driver (eg. i915) supports avsink, it can create 
   a display client, add power wells and display
 outputs to the client, and then register the display client to the 
   avsink core. Then it may look up if there is any
 audio client registered, by name or type, and may find an audio client 
   registered by some audio driver.
   
   * If an audio driver supports avsink, it usually should look up a 
   registered display client by name or type at first,
 because it may need the shared power well in GPU and check the display 
   outputs' name to bind the audio inputs. If
 the display client is not registered yet, the audio driver can choose 
   to wait (maybe in a work queue) or return
 -EAGAIN for a deferred probe. After the display client is found, the 
   audio driver can register an audio client with

-EPROBE_DEFER is the correct error code for deferred probing.

   6. Display register operation (optional)
   
   Some audio driver needs to access GPU audio registers. The register ops 
   are provided by the peer display client.
   
   struct avsink_registers_ops {
int (*read_register) (uint32_t reg_addr, uint32_t *data, void 
   *context);
int (*write_register) (uint32_t reg_addr, uint32_t data, void 
   *context);
int (*read_modify_register) (uint32_t reg_addr, uint32_t data, 
   uint32_t mask, void *context);
   
   int avsink_define_reg_ops (struct avsink_client *client, struct 
   avsink_registers_ops *ops);
   
   And avsink core provides API for the audio driver to access the display 
   registers:
   
   int avsink_read_display_register(struct avsink_client *client , 

Re: [PATCH] Documentation/dma-buf-sharing.txt: update API descriptions

[Thierry Reding]

On Mon, May 12, 2014 at 08:48:12PM +0900, gioh.kim wrote:
 From: gioh.kim gioh@lge.com

It might be good to fix your setup to make this be the same as the name
and email used in the Signed-off-by line below.

 update some descriptions for API arguments and descriptions.

Nit: Update since it's the beginning of a sentence.

 Signed-off-by: Gioh Kim gioh@lge.com
 ---
  Documentation/dma-buf-sharing.txt |   10 ++
  1 file changed, 6 insertions(+), 4 deletions(-)
 
 diff --git a/Documentation/dma-buf-sharing.txt 
 b/Documentation/dma-buf-sharing.txt
 index 505e711..1ea89b8 100644
 --- a/Documentation/dma-buf-sharing.txt
 +++ b/Documentation/dma-buf-sharing.txt
 @@ -56,7 +56,7 @@ The dma_buf buffer sharing API usage contains the following 
 steps:
size_t size, int flags,
const char *exp_name)
  
 -   If this succeeds, dma_buf_export allocates a dma_buf structure, and 
 returns a
 +   If this succeeds, dma_buf_export_named allocates a dma_buf structure, and 
 returns a

Perhaps reformat this so that the lines don't exceed 80 characters?

 pointer to the same. It also associates an anonymous file with this 
 buffer,
 so it can be exported. On failure to allocate the dma_buf object, it 
 returns
 NULL.
 @@ -66,7 +66,7 @@ The dma_buf buffer sharing API usage contains the following 
 steps:
  
 Exporting modules which do not wish to provide any specific name may use 
 the
 helper define 'dma_buf_export()', with the same arguments as above, but
 -   without the last argument; a __FILE__ pre-processor directive will be
 +   without the last argument; a KBUILD_MODNAME pre-processor directive will 
 be
 inserted in place of 'exp_name' instead.

This was already fixed in commit 2e33def0339c (dma-buf: update exp_name
when using dma_buf_export()). Perhaps you should rebase this patch on
top of the latest linux-next.

Otherwise looks good.

Thierry


pgpyjC7DMFPRv.pgp
Description: PGP signature

Re: [PATCH v12][ 09/12] drm/panel: Add Eukrea mbimxsd51 displays.

[Thierry Reding]

On Mon, Apr 07, 2014 at 02:44:48PM +0200, Denis Carikli wrote:
[...]
 +static const struct panel_desc eukrea_mbimxsd51_dvisvga = {
 + .modes = eukrea_mbimxsd51_dvisvga_mode,
 + .num_modes = 1,
 + .size = {
 + .width = 0,
 + .height = 0,
 + },
 +};
[...]
 +static const struct panel_desc eukrea_mbimxsd51_dvivga = {
 + .modes = eukrea_mbimxsd51_dvivga_mode,
 + .num_modes = 1,
 + .size = {
 + .width = 0,
 + .height = 0,
 + },
 +};

Surely these two panels have a physical size?

Thierry


pgprwiymAtwfE.pgp
Description: PGP signature

Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Thierry Reding]

On Fri, Feb 07, 2014 at 04:55:00PM +0100, Jean-Francois Moine wrote:
 Some simple components don't need to do any specific action on
 bind to / unbind from a master component.
 
 This patch permits such components to omit the bind/unbind
 operations.
 
 Signed-off-by: Jean-Francois Moine moin...@free.fr
 ---
  drivers/base/component.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/base/component.c b/drivers/base/component.c
 index c53efe6..0a39d7a 100644
 --- a/drivers/base/component.c
 +++ b/drivers/base/component.c
 @@ -225,7 +225,8 @@ static void component_unbind(struct component *component,
  {
   WARN_ON(!component-bound);
  
 - component-ops-unbind(component-dev, master-dev, data);
 + if (component-ops)
 + component-ops-unbind(component-dev, master-dev, data);

This doesn't actually do what the commit message says. This makes
component-ops optional, not component-ops-unbind().

A more correct check would be:

if (component-ops  component-ops-unbind)

   component-bound = false;
  
   /* Release all resources claimed in the binding of this component */
 @@ -274,7 +275,11 @@ static int component_bind(struct component *component, 
 struct master *master,
   dev_dbg(master-dev, binding %s (ops %ps)\n,
   dev_name(component-dev), component-ops);
  
 - ret = component-ops-bind(component-dev, master-dev, data);
 + if (component-ops)
 + ret = component-ops-bind(component-dev, master-dev, data);

Same here.

Thierry


pgplQgNSzd8Tu.pgp
Description: PGP signature

Re: [PATCHv5][ 3/8] staging: imx-drm: Correct BGR666 and the board's dts that use them.

[Thierry Reding]

On Thu, Dec 05, 2013 at 07:28:07PM +0100, Denis Carikli wrote:
[...]
 diff --git a/arch/arm/boot/dts/imx51-apf51dev.dts 
 b/arch/arm/boot/dts/imx51-apf51dev.dts
 index f36a3aa..3b6de6a 100644
 --- a/arch/arm/boot/dts/imx51-apf51dev.dts
 +++ b/arch/arm/boot/dts/imx51-apf51dev.dts
 @@ -19,7 +19,7 @@
   display@di1 {
   

I know this isn't introduced by your patch, but WTF???

Thierry


pgpC0LG2R1tDP.pgp
Description: PGP signature

Re: [PATCHv5][ 2/8] staging: imx-drm: Add RGB666 support for parallel display.

[Thierry Reding]

On Thu, Dec 05, 2013 at 07:28:06PM +0100, Denis Carikli wrote:
[...]
 diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c 
 b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c
[...]
 @@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt)
   return IPU_DC_MAP_BGR666;
   case V4L2_PIX_FMT_BGR24:
   return IPU_DC_MAP_BGR24;
 + case V4L2_PIX_FMT_RGB666:
 + return IPU_DC_MAP_RGB666;

Why is this DRM driver even using V4L2 pixel formats in the first place?

Thierry


pgpCx5QbEe39s.pgp
Description: PGP signature

Re: [PATCHv5][ 2/8] staging: imx-drm: Add RGB666 support for parallel display.

[Thierry Reding]

On Fri, Dec 06, 2013 at 02:29:22PM +0100, Lucas Stach wrote:
 Am Freitag, den 06.12.2013, 14:14 +0100 schrieb Thierry Reding:
  On Thu, Dec 05, 2013 at 07:28:06PM +0100, Denis Carikli wrote:
  [...]
   diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c 
   b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c
  [...]
   @@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt)
 return IPU_DC_MAP_BGR666;
 case V4L2_PIX_FMT_BGR24:
 return IPU_DC_MAP_BGR24;
   + case V4L2_PIX_FMT_RGB666:
   + return IPU_DC_MAP_RGB666;
  
  Why is this DRM driver even using V4L2 pixel formats in the first place?
  
 Because imx-drm is actually a misnomer. The i.MX IPU is a multifunction
 device, which as one part has the display controllers, but also camera
 interfaces and mem-to-mem scaler devices, which are hooked up via the
 V4L2 interface.
 
 The generic IPU part, which is used for example for programming the DMA
 channels is using V4L2 pixel formats as a common base. We have patches
 to split this out and make this fact more visible. (The IPU core will be
 placed aside the Tegra host1x driver)

Have you considered splitting thing up further and move out the display
controller driver to DRM and the camera driver to V4L2? I mean, if that
is even possible with a reasonable amount of work.

Is the mem-to-mem the same as the DMA channels you mentioned? If it
only does DMA, why does it even need to worry about pixel formats?

Thierry


pgpDJUGj56GQ6.pgp
Description: PGP signature

Re: V2: Agenda for the Edinburgh mini-summit

[Thierry Reding]

On Mon, Sep 23, 2013 at 10:27:06PM +0200, Sylwester Nawrocki wrote:
 On 09/23/2013 06:37 PM, Oliver Schinagl wrote:
 On 09/23/13 16:45, Sylwester Nawrocki wrote:
 Hi,
 
 I would like to have a short discussion on LED flash devices support
 in the kernel. Currently there are two APIs: the V4L2 and LED class
 API exposed by the kernel, which I believe is not good from user space
 POV. Generic applications will need to implement both APIs. I think we
 should decide whether to extend the led class API to add support for
 more advanced LED controllers there or continue to use the both APIs
 with overlapping functionality.
 There has been some discussion about this on the ML, but without any
 consensus reached [1].
 
 What about the linux-pwm framework and its support for the backlight via
 dts?
 
 Or am I talking way to uninformed here. Copying backlight to flashlight
 with some minor modification sounds sensible in a way...
 
 I'd assume we don't need yet another user interface for the LEDs ;) AFAICS
 the PWM subsystem exposes pretty much raw interface in sysfs. The PWM LED
 controllers are already handled in the leds-class API, there is the
 leds_pwm
 driver (drivers/leds/leds-pwm.c).
 
 I'm adding linux-pwm and linux-leds maintainers at Cc so someone may correct
 me if I got anything wrong.

The PWM subsystem is most definitely not a good fit for this. The only
thing it provides is a way for other drivers to access a PWM device and
use it for some specific purpose (pwm-backlight, leds-pwm).

The sysfs support is a convenience for people that needs to use a PWM in
a way for which no driver framework exists, or for which it doesn't make
sense to write a driver. Or for testing.

 Presumably, what we need is a few enhancements to support in a standard way
 devices like MAX77693, LM3560 or MAX8997.  There is already a led
 class driver
 for the MAX8997 LED controller (drivers/leds/leds-max8997.c), but it
 uses some
 device-specific sysfs attributes.
 
 Thus similar devices are currently being handled by different subsystems.
 The split between the V4L2 Flash and the leds class API WRT to Flash LED
 controller drivers is included in RFC [1], it seems still up to date.
 
 
 [1] http://www.spinics.net/lists/linux-leds/msg00899.html

Perhaps it would make sense for V4L2 to be able to use a LED as exposed
by the LED subsystem and wrap it so that it can be integrated with V4L2?
If functionality is missing from the LED subsystem I suppose that could
be added.

If I understand correctly, the V4L2 subsystem uses LEDs as flashes for
camera devices. I can easily imagine that there are devices out there
which provide functionality beyond what a regular LED will provide. So
perhaps for things such as mobile phones, which typically use a plain
LED to illuminate the surroundings, an LED wrapped into something that
emulates the flash functionality could work. But I doubt that the LED
subsystem is a good fit for anything beyond that.

Thierry


pgp5r15Q9vljn.pgp
Description: PGP signature

Re: V2: Agenda for the Edinburgh mini-summit

[Thierry Reding]

Resending using Bryan's correct email address.

On Mon, Sep 23, 2013 at 10:27:06PM +0200, Sylwester Nawrocki wrote:
 On 09/23/2013 06:37 PM, Oliver Schinagl wrote:
 On 09/23/13 16:45, Sylwester Nawrocki wrote:
 Hi,
 
 I would like to have a short discussion on LED flash devices support
 in the kernel. Currently there are two APIs: the V4L2 and LED class
 API exposed by the kernel, which I believe is not good from user space
 POV. Generic applications will need to implement both APIs. I think we
 should decide whether to extend the led class API to add support for
 more advanced LED controllers there or continue to use the both APIs
 with overlapping functionality.
 There has been some discussion about this on the ML, but without any
 consensus reached [1].
 
 What about the linux-pwm framework and its support for the backlight via
 dts?
 
 Or am I talking way to uninformed here. Copying backlight to flashlight
 with some minor modification sounds sensible in a way...
 
 I'd assume we don't need yet another user interface for the LEDs ;) AFAICS
 the PWM subsystem exposes pretty much raw interface in sysfs. The PWM LED
 controllers are already handled in the leds-class API, there is the
 leds_pwm
 driver (drivers/leds/leds-pwm.c).
 
 I'm adding linux-pwm and linux-leds maintainers at Cc so someone may correct
 me if I got anything wrong.

The PWM subsystem is most definitely not a good fit for this. The only
thing it provides is a way for other drivers to access a PWM device and
use it for some specific purpose (pwm-backlight, leds-pwm).

The sysfs support is a convenience for people that needs to use a PWM in
a way for which no driver framework exists, or for which it doesn't make
sense to write a driver. Or for testing.

 Presumably, what we need is a few enhancements to support in a standard way
 devices like MAX77693, LM3560 or MAX8997.  There is already a led
 class driver
 for the MAX8997 LED controller (drivers/leds/leds-max8997.c), but it
 uses some
 device-specific sysfs attributes.
 
 Thus similar devices are currently being handled by different subsystems.
 The split between the V4L2 Flash and the leds class API WRT to Flash LED
 controller drivers is included in RFC [1], it seems still up to date.
 
 
 [1] http://www.spinics.net/lists/linux-leds/msg00899.html

Perhaps it would make sense for V4L2 to be able to use a LED as exposed
by the LED subsystem and wrap it so that it can be integrated with V4L2?
If functionality is missing from the LED subsystem I suppose that could
be added.

If I understand correctly, the V4L2 subsystem uses LEDs as flashes for
camera devices. I can easily imagine that there are devices out there
which provide functionality beyond what a regular LED will provide. So
perhaps for things such as mobile phones, which typically use a plain
LED to illuminate the surroundings, an LED wrapped into something that
emulates the flash functionality could work. But I doubt that the LED
subsystem is a good fit for anything beyond that.

Thierry


pgpLiep3md6DX.pgp
Description: PGP signature

Re: [PATCH RESEND] i2c: move of helpers into the core

[Thierry Reding]

On Mon, Aug 19, 2013 at 07:59:40PM +0200, Wolfram Sang wrote:
[...]
 diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
[...]
 +#if IS_ENABLED(CONFIG_OF)
 +static void of_i2c_register_devices(struct i2c_adapter *adap)
 +{
[...]
 +}
[...]
 +#endif /* CONFIG_OF */

Isn't this missing the dummy implementation for !OF.

  static int i2c_do_add_adapter(struct i2c_driver *driver,
 struct i2c_adapter *adap)
  {
 @@ -1058,6 +1160,8 @@ static int i2c_register_adapter(struct i2c_adapter 
 *adap)
  
  exit_recovery:
   /* create pre-declared device nodes */
 + of_i2c_register_devices(adap);

Alternatively you could remove the of_i2c_register_devices() from the
#ifdef CONFIG_OF block so that it will always be compiled. You could
turn the above into

if (IS_ENABLED(CONFIG_OF))
of_i2c_register_devices(adap);

and let the compiler throw the static function away if it sees that the
condition is always false.

Thierry


pgpw1yWn8EA6p.pgp
Description: PGP signature

Re: [PATCH 1/4] [media] sh_veu.c: Convert to devm_ioremap_resource()

[Thierry Reding]

On Mon, Mar 04, 2013 at 01:45:18PM +0530, Sachin Kamat wrote:
 Use the newly introduced devm_ioremap_resource() instead of
 devm_request_and_ioremap() which provides more consistent error handling.
 
 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 ---
  drivers/media/platform/sh_veu.c |7 ---
  1 files changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
 index cb54c69..362d88e 100644
 --- a/drivers/media/platform/sh_veu.c
 +++ b/drivers/media/platform/sh_veu.c
 @@ -10,6 +10,7 @@
   * published by the Free Software Foundation
   */
  
 +#include linux/err.h
  #include linux/fs.h
  #include linux/kernel.h
  #include linux/module.h
 @@ -1164,9 +1165,9 @@ static int sh_veu_probe(struct platform_device *pdev)
  
   veu-is_2h = resource_size(reg_res) == 0x22c;
  
 - veu-base = devm_request_and_ioremap(pdev-dev, reg_res);
 - if (!veu-base)
 - return -ENOMEM;
 + veu-base = devm_ioremap_resource(pdev-dev, reg_res);
 + if (IS_ERR(veu-base))
 + return PTR_ERR(veu-base);
  
   ret = devm_request_threaded_irq(pdev-dev, irq, sh_veu_isr, sh_veu_bh,
   0, veu, veu);

Reviewed-by: Thierry Reding thierry.red...@avionic-design.de


pgpj_jhgxSGrB.pgp
Description: PGP signature

Re: [PATCH 2/4] [media] soc_camera/pxa_camera: Convert to devm_ioremap_resource()

[Thierry Reding]

On Mon, Mar 04, 2013 at 01:45:19PM +0530, Sachin Kamat wrote:
 Use the newly introduced devm_ioremap_resource() instead of
 devm_request_and_ioremap() which provides more consistent error handling.
 
 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 ---
  drivers/media/platform/soc_camera/pxa_camera.c |8 +---
  1 files changed, 5 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/pxa_camera.c 
 b/drivers/media/platform/soc_camera/pxa_camera.c
 index 395e2e0..42abbce 100644
 --- a/drivers/media/platform/soc_camera/pxa_camera.c
 +++ b/drivers/media/platform/soc_camera/pxa_camera.c
 @@ -15,6 +15,7 @@
  #include linux/io.h
  #include linux/delay.h
  #include linux/dma-mapping.h
 +#include linux/err.h
  #include linux/errno.h
  #include linux/fs.h
  #include linux/interrupt.h
 @@ -1710,9 +1711,10 @@ static int pxa_camera_probe(struct platform_device 
 *pdev)
   /*
* Request the regions.
*/
 - base = devm_request_and_ioremap(pdev-dev, res);
 - if (!base)
 - return -ENOMEM;
 + base = devm_ioremap_resource(pdev-dev, res);
 + if (IS_ERR(base))
 + return PTR_ERR(base);
 +
   pcdev-irq = irq;
   pcdev-base = base;
  

Reviewed-by: Thierry Reding thierry.red...@avionic-design.de


pgpTZnjrAwC5p.pgp
Description: PGP signature

Re: [PATCH 3/4] [media] soc_camera/sh_mobile_ceu_camera: Convert to devm_ioremap_resource()

[Thierry Reding]

On Mon, Mar 04, 2013 at 01:45:20PM +0530, Sachin Kamat wrote:
 Use the newly introduced devm_ioremap_resource() instead of
 devm_request_and_ioremap() which provides more consistent error handling.
 
 devm_ioremap_resource() provides its own error messages; so all explicit
 error messages can be removed from the failure code paths.
 
 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 ---
  .../platform/soc_camera/sh_mobile_ceu_camera.c |9 -
  1 files changed, 4 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c 
 b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
 index bb08a46..be1af08 100644
 --- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
 +++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
 @@ -20,6 +20,7 @@
  #include linux/completion.h
  #include linux/delay.h
  #include linux/dma-mapping.h
 +#include linux/err.h
  #include linux/errno.h
  #include linux/fs.h
  #include linux/interrupt.h
 @@ -2110,11 +2111,9 @@ static int sh_mobile_ceu_probe(struct platform_device 
 *pdev)
   pcdev-max_width = pcdev-pdata-max_width ? : 2560;
   pcdev-max_height = pcdev-pdata-max_height ? : 1920;
  
 - base = devm_request_and_ioremap(pdev-dev, res);
 - if (!base) {
 - dev_err(pdev-dev, Unable to ioremap CEU registers.\n);
 - return -ENXIO;
 - }
 + base = devm_ioremap_resource(pdev-dev, res);
 + if (IS_ERR(base))
 + return PTR_ERR(base);
  
   pcdev-irq = irq;
   pcdev-base = base;

Reviewed-by: Thierry Reding thierry.red...@avionic-design.de


pgpFpVhGCuXDK.pgp
Description: PGP signature

Re: [PATCH 4/4] [media] soc_camera/sh_mobile_csi2: Convert to devm_ioremap_resource()

[Thierry Reding]

On Mon, Mar 04, 2013 at 01:45:21PM +0530, Sachin Kamat wrote:
 Use the newly introduced devm_ioremap_resource() instead of
 devm_request_and_ioremap() which provides more consistent error handling.
 
 devm_ioremap_resource() provides its own error messages; so all explicit
 error messages can be removed from the failure code paths.
 
 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 ---
  drivers/media/platform/soc_camera/sh_mobile_csi2.c |9 -
  1 files changed, 4 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/sh_mobile_csi2.c 
 b/drivers/media/platform/soc_camera/sh_mobile_csi2.c
 index 42c559e..09cb4fc 100644
 --- a/drivers/media/platform/soc_camera/sh_mobile_csi2.c
 +++ b/drivers/media/platform/soc_camera/sh_mobile_csi2.c
 @@ -9,6 +9,7 @@
   */
  
  #include linux/delay.h
 +#include linux/err.h
  #include linux/i2c.h
  #include linux/io.h
  #include linux/platform_device.h
 @@ -324,11 +325,9 @@ static int sh_csi2_probe(struct platform_device *pdev)
  
   priv-irq = irq;
  
 - priv-base = devm_request_and_ioremap(pdev-dev, res);
 - if (!priv-base) {
 - dev_err(pdev-dev, Unable to ioremap CSI2 registers.\n);
 - return -ENXIO;
 - }
 + priv-base = devm_ioremap_resource(pdev-dev, res);
 + if (IS_ERR(priv-base))
 + return PTR_ERR(priv-base);
  
   priv-pdev = pdev;
   platform_set_drvdata(pdev, priv);

Reviewed-by: Thierry Reding thierry.red...@avionic-design.de


pgpeYgw2RPUw8.pgp
Description: PGP signature

Re: [git:v4l-dvb/for_v3.9] [media] media: Convert to devm_ioremap_resource()

[Thierry Reding]

On Wed, Feb 06, 2013 at 12:33:04PM +0100, Mauro Carvalho Chehab wrote:
 This is an automatic generated email to let you know that the following patch 
 were queued at the 
 http://git.linuxtv.org/media_tree.git tree:
 
 Subject: [media] media: Convert to devm_ioremap_resource()
 Author:  Thierry Reding thierry.red...@avionic-design.de
 Date:Mon Jan 21 06:09:07 2013 -0300
 
 Convert all uses of devm_request_and_ioremap() to the newly introduced
 devm_ioremap_resource() which provides more consistent error handling.
 devm_ioremap_resource() provides its own error messages so all explicit
 error messages can be removed from the failure code paths.
 
 Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
 Acked-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com

Hi Mauro,

Greg already took this through the driver-core tree because of the
dependencies, so I don't think it's necessary to have it in yours as
well.

Thierry


pgpwHHYjgnziw.pgp
Description: PGP signature

[PATCH 14/33] media: Convert to devm_ioremap_resource()

[Thierry Reding]

Convert all uses of devm_request_and_ioremap() to the newly introduced
devm_ioremap_resource() which provides more consistent error handling.

devm_ioremap_resource() provides its own error messages so all explicit
error messages can be removed from the failure code paths.

Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-media@vger.kernel.org
---
 drivers/media/platform/exynos-gsc/gsc-core.c   |  8 +++-
 drivers/media/platform/mx2_emmaprp.c   |  6 +++---
 drivers/media/platform/s3c-camif/camif-core.c  |  8 +++-
 drivers/media/platform/s5p-fimc/fimc-core.c|  8 +++-
 drivers/media/platform/s5p-fimc/fimc-lite.c|  8 +++-
 drivers/media/platform/s5p-fimc/mipi-csis.c|  8 +++-
 drivers/media/platform/s5p-g2d/g2d.c   |  8 +++-
 drivers/media/platform/s5p-jpeg/jpeg-core.c|  8 +++-
 drivers/media/platform/s5p-mfc/s5p_mfc.c   |  8 +++-
 drivers/media/platform/soc_camera/mx2_camera.c | 12 ++--
 10 files changed, 33 insertions(+), 49 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 2b1b9f3..c1a0713 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1098,11 +1098,9 @@ static int gsc_probe(struct platform_device *pdev)
mutex_init(gsc-lock);
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-   gsc-regs = devm_request_and_ioremap(dev, res);
-   if (!gsc-regs) {
-   dev_err(dev, failed to map registers\n);
-   return -ENOENT;
-   }
+   gsc-regs = devm_ioremap_resource(dev, res);
+   if (IS_ERR(gsc-regs))
+   return PTR_ERR(gsc-regs);
 
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!res) {
diff --git a/drivers/media/platform/mx2_emmaprp.c 
b/drivers/media/platform/mx2_emmaprp.c
index 6b155d7..4b9e0a2 100644
--- a/drivers/media/platform/mx2_emmaprp.c
+++ b/drivers/media/platform/mx2_emmaprp.c
@@ -941,9 +941,9 @@ static int emmaprp_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, pcdev);
 
-   pcdev-base_emma = devm_request_and_ioremap(pdev-dev, res_emma);
-   if (!pcdev-base_emma) {
-   ret = -ENXIO;
+   pcdev-base_emma = devm_ioremap_resource(pdev-dev, res_emma);
+   if (IS_ERR(pcdev-base_emma)) {
+   ret = PTR_ERR(pcdev-base_emma);
goto rel_vdev;
}
 
diff --git a/drivers/media/platform/s3c-camif/camif-core.c 
b/drivers/media/platform/s3c-camif/camif-core.c
index e2716c3..09a8c9c 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -433,11 +433,9 @@ static int s3c_camif_probe(struct platform_device *pdev)
 
mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-   camif-io_base = devm_request_and_ioremap(dev, mres);
-   if (!camif-io_base) {
-   dev_err(dev, failed to obtain I/O memory\n);
-   return -ENOENT;
-   }
+   camif-io_base = devm_ioremap_resource(dev, mres);
+   if (IS_ERR(camif-io_base))
+   return PTR_ERR(camif-io_base);
 
ret = camif_request_irqs(pdev, camif);
if (ret  0)
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c 
b/drivers/media/platform/s5p-fimc/fimc-core.c
index 545b46a..acc0f84 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -909,11 +909,9 @@ static int fimc_probe(struct platform_device *pdev)
mutex_init(fimc-lock);
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-   fimc-regs = devm_request_and_ioremap(pdev-dev, res);
-   if (fimc-regs == NULL) {
-   dev_err(pdev-dev, Failed to obtain io memory\n);
-   return -ENOENT;
-   }
+   fimc-regs = devm_ioremap_resource(pdev-dev, res);
+   if (IS_ERR(fimc-regs))
+   return PTR_ERR(fimc-regs);
 
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL) {
diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c 
b/drivers/media/platform/s5p-fimc/fimc-lite.c
index ed67220..67db9f8 100644
--- a/drivers/media/platform/s5p-fimc/fimc-lite.c
+++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
@@ -1426,11 +1426,9 @@ static int fimc_lite_probe(struct platform_device *pdev)
mutex_init(fimc-lock);
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-   fimc-regs = devm_request_and_ioremap(pdev-dev, res);
-   if (fimc-regs == NULL) {
-   dev_err(pdev-dev, Failed to obtain io memory\n);
-   return -ENOENT;
-   }
+   fimc-regs = devm_ioremap_resource(pdev-dev, res);
+   if (IS_ERR(fimc-regs))
+   return PTR_ERR(fimc-regs);
 
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL

Re: [PATCHv16 0/7] of: add display helper

[Thierry Reding]

On Wed, Jan 09, 2013 at 09:15:41PM +0100, Steffen Trumtrar wrote:
 On Tue, Dec 18, 2012 at 06:04:09PM +0100, Steffen Trumtrar wrote:
  Hi!
  
  Finally, right in time before the end of the world on friday, v16 of the
  display helpers.
  
 
 So, any more criticism on the series? Any takers for the series as is?
 I guess it could be merged via the fbdev-tree if David Airlie can agree
 to the DRM patches ?! Does that sound about right?
 
 I think the series was tested at least with
   - imx6q: sabrelite, sabresd
   - imx53: tqma53/mba53
   - omap: DA850 EVM, AM335x EVM, EVM-SK
 
 I don't know what Laurent Pinchart, Marek Vasut and Leela Krishna Amudala
 are using. Those are the people I know from the top of my head, that use
 or at least did use the patches in one of its iterations. If I forgot
 anyone, please speak up and possibly add your new HW to the list of tested
 devices.

I tested earlier versions on Tegra. The latest one was v15 I think.

Thierry


pgpNAKduiOz6G.pgp
Description: PGP signature

Re: [RFC v2 0/5] Common Display Framework

[Thierry Reding]

On Thu, Nov 22, 2012 at 10:45:31PM +0100, Laurent Pinchart wrote:
[...]
 Display entities are accessed by driver using notifiers. Any driver can
 register a display entity notifier with the CDF, which then calls the notifier
 when a matching display entity is registered. The reason for this asynchronous
 mode of operation, compared to how drivers acquire regulator or clock
 resources, is that the display entities can use resources provided by the
 display driver. For instance a panel can be a child of the DBI or DSI bus
 controlled by the display device, or use a clock provided by that device. We
 can't defer the display device probe until the panel is registered and also
 defer the panel device probe until the display is registered. As most display
 drivers need to handle output devices hotplug (HDMI monitors for instance),
 handling other display entities through a notification system seemed to be the
 easiest solution.
 
 Note that this brings a different issue after registration, as display
 controller and display entity drivers would take a reference to each other.
 Those circular references would make driver unloading impossible. One possible
 solution to this problem would be to simulate an unplug event for the display
 entity, to force the display driver to release the dislay entities it uses. We
 would need a userspace API for that though. Better solutions would of course
 be welcome.

Maybe I don't understand all of the underlying issues correctly, but a
parent/child model would seem like a better solution to me. We discussed
this back when designing the DT bindings for Tegra DRM and came to the
conclusion that the output resource of the display controller (RGB,
HDMI, DSI or TVO) was the most suitable candidate to be the parent of
the panel or display attached to it. The reason for that decision was
that it keeps the flow of data or addressing of nodes consistent. So the
chain would look something like this (on Tegra):

CPU
+-host1x
  +-dc
+-rgb
| +-panel
+-hdmi
  +-monitor

In a natural way this makes the output resource the master of the panel
or display. From a programming point of view this becomes quite easy to
implement and is very similar to how other busses like I2C or SPI are
modelled. In device tree these would be represented as subnodes, while
with platform data some kind of lookup could be done like for regulators
or alternatively a board setup registration mechanism like what's in
place for I2C or SPI.

Thierry


pgpRbeGtDw3N5.pgp
Description: PGP signature

Re: [PATCH v12 2/6] video: add of helper for videomode

[Thierry Reding]

On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
 Hi!
 
 On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
  Hi Steffen,
  
  On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
   +/**
   + * of_get_display_timings - parse all display_timing entries from a 
   device_node
   + * @np: device_node with the subnodes
   + **/
   +struct display_timings *of_get_display_timings(const struct device_node 
   *np)
   +{
   + struct device_node *timings_np;
   + struct device_node *entry;
   + struct device_node *native_mode;
   + struct display_timings *disp;
   +
   + if (!np) {
   + pr_err(%s: no devicenode given\n, __func__);
   + return NULL;
   + }
   +
   + timings_np = of_find_node_by_name(np, display-timings);
  
  I get below build warnings on this line
  drivers/video/of_display_timing.c: In function 'of_get_display_timings':
  drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 
  'of_find_node_by_name' discards qualifiers from pointer target type
  include/linux/of.h:167:28: note: expected 'struct device_node *' but 
  argument is of type 'const struct device_node *'
  
   + * of_display_timings_exists - check if a display-timings node is 
   provided
   + * @np: device_node with the timing
   + **/
   +int of_display_timings_exists(const struct device_node *np)
   +{
   + struct device_node *timings_np;
   +
   + if (!np)
   + return -EINVAL;
   +
   + timings_np = of_parse_phandle(np, display-timings, 0);
  
  Also here:
  drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
  drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 
  'of_parse_phandle' discards qualifiers from pointer target type
  include/linux/of.h:258:28: note: expected 'struct device_node *' but 
  argument is of type 'const struct device_node *'
  
 
 The warnings are because the of-functions do not use const pointers where they
 should. I had two options: don't use const pointers even if they should be and
 have no warnings or use const pointers and have a correct API. (Third option:
 send patches for of-functions). I chose the second option.

Maybe a better approach would be a combination of 1 and 3: don't use
const pointers for struct device_node for now and bring the issue up
with the OF maintainers, possibly with patches attached that fix the
problematic functions.

Thierry


pgpy9GlPQOvHD.pgp
Description: PGP signature

Re: [PATCH v12 2/6] video: add of helper for videomode

[Thierry Reding]

On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
 On 11/21/2012 05:52 AM, Thierry Reding wrote:
  On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
  Hi!
 
  On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
  Hi Steffen,
 
  On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
  +/**
  + * of_get_display_timings - parse all display_timing entries from a 
  device_node
  + * @np: device_node with the subnodes
  + **/
  +struct display_timings *of_get_display_timings(const struct device_node 
  *np)
  +{
  +struct device_node *timings_np;
  +struct device_node *entry;
  +struct device_node *native_mode;
  +struct display_timings *disp;
  +
  +if (!np) {
  +pr_err(%s: no devicenode given\n, __func__);
  +return NULL;
  +}
  +
  +timings_np = of_find_node_by_name(np, display-timings);
 
  I get below build warnings on this line
  drivers/video/of_display_timing.c: In function 'of_get_display_timings':
  drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 
  'of_find_node_by_name' discards qualifiers from pointer target type
  include/linux/of.h:167:28: note: expected 'struct device_node *' but 
  argument is of type 'const struct device_node *'
 
  + * of_display_timings_exists - check if a display-timings node is 
  provided
  + * @np: device_node with the timing
  + **/
  +int of_display_timings_exists(const struct device_node *np)
  +{
  +struct device_node *timings_np;
  +
  +if (!np)
  +return -EINVAL;
  +
  +timings_np = of_parse_phandle(np, display-timings, 0);
 
  Also here:
  drivers/video/of_display_timing.c: In function 
  'of_display_timings_exists':
  drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 
  'of_parse_phandle' discards qualifiers from pointer target type
  include/linux/of.h:258:28: note: expected 'struct device_node *' but 
  argument is of type 'const struct device_node *'
 
 
  The warnings are because the of-functions do not use const pointers where 
  they
  should. I had two options: don't use const pointers even if they should be 
  and
  have no warnings or use const pointers and have a correct API. (Third 
  option:
  send patches for of-functions). I chose the second option.
  
  Maybe a better approach would be a combination of 1 and 3: don't use
  const pointers for struct device_node for now and bring the issue up
  with the OF maintainers, possibly with patches attached that fix the
  problematic functions.
 
 Why does this need to be const? Since some DT functions increment
 refcount the node, I'm not sure that making struct device_node const in
 general is right thing to do. I do think it should be okay for
 of_parse_phandle.

I wasn't proposing to do it everywhere but only where possible. If the
node is modified in some way then obviously it shouldn't be const.

Thierry


pgpzfOis2Qo8e.pgp
Description: PGP signature

Re: [PATCH v12 0/6] of: add display helper

[Thierry Reding]

On Tue, Nov 20, 2012 at 07:11:29PM +0100, Robert Schwebel wrote:
 On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
  On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
   Hi!
   
   Changes since v11:
 - make pointers const where applicable
 - add reviewed-by Laurent Pinchart
  
  Looks good to me.
  
  Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
  
  Through which tree do you plan to push this ?
 
 We have no idea yet, and none of the people on Cc: have volunteered
 so far... what do you think?

The customary approach would be to take the patches through separate
trees, but I think this particular series is sufficiently interwoven to
warrant taking them all through one tree. However the least that we
should do is collect Acked-by's from the other tree maintainers.

Given that most of the patches modify files in drivers/video, Florian's
fbdev tree would be the most obvious candidate, right? If Florian agrees
to take the patches, all we would need is David's Acked-by.

How does that sound?

Thierry


pgpKLIDOP7HB1.pgp
Description: PGP signature

Re: [PATCH v10 6/6] drm_modes: add of_videomode helpers

[Thierry Reding]

On Thu, Nov 15, 2012 at 10:23:57AM +0100, Steffen Trumtrar wrote:
[...]
 +int of_get_drm_display_mode(struct device_node *np,
 + struct drm_display_mode *dmode, unsigned int index)
 +{
 + struct videomode vm;
 + int ret;
 +
 + ret = of_get_videomode(np, vm, index);
 + if (ret)
 + return ret;
 +
 + display_mode_from_videomode(vm, dmode);

This function is now called drm_display_mode_from_videomode().

Thierry


pgpND3qQBeg7C.pgp
Description: PGP signature

Re: [PATCH v10 0/6] of: add display helper

[Thierry Reding]

On Thu, Nov 15, 2012 at 10:23:51AM +0100, Steffen Trumtrar wrote:
 Hi!
 
 Changes since v9:
   - don't leak memory when previous timings were correct
   - CodingStyle fixes
   - move blank lines around
 
 Regards,
 Steffen
 
 
 Steffen Trumtrar (6):
   video: add display_timing and videomode
   video: add of helper for videomode
   fbmon: add videomode helpers
   fbmon: add of_videomode helpers
   drm_modes: add videomode helpers
   drm_modes: add of_videomode helpers
 
  .../devicetree/bindings/video/display-timings.txt  |  107 ++
  drivers/gpu/drm/drm_modes.c|   70 +++
  drivers/video/Kconfig  |   19 ++
  drivers/video/Makefile |4 +
  drivers/video/display_timing.c |   24 +++
  drivers/video/fbmon.c  |   86 
  drivers/video/of_display_timing.c  |  212 
 
  drivers/video/of_videomode.c   |   47 +
  drivers/video/videomode.c  |   45 +
  include/drm/drmP.h |   12 ++
  include/linux/display_timing.h |   69 +++
  include/linux/fb.h |   12 ++
  include/linux/of_display_timings.h |   20 ++
  include/linux/of_videomode.h   |   17 ++
  include/linux/videomode.h  |   40 
  15 files changed, 784 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/video/display-timings.txt
  create mode 100644 drivers/video/display_timing.c
  create mode 100644 drivers/video/of_display_timing.c
  create mode 100644 drivers/video/of_videomode.c
  create mode 100644 drivers/video/videomode.c
  create mode 100644 include/linux/display_timing.h
  create mode 100644 include/linux/of_display_timings.h
  create mode 100644 include/linux/of_videomode.h
  create mode 100644 include/linux/videomode.h

With the one change that I pointed out, the whole series:

Reviewed-by: Thierry Reding thierry.red...@avionic-design.de
Acked-by: Thierry Reding thierry.red...@avionic-design.de


pgpy52p9Speq9.pgp
Description: PGP signature

Re: [PATCH v10 0/6] of: add display helper

[Thierry Reding]

On Thu, Nov 15, 2012 at 11:24:11AM +0100, Thierry Reding wrote:
 On Thu, Nov 15, 2012 at 10:23:51AM +0100, Steffen Trumtrar wrote:
  Hi!
  
  Changes since v9:
  - don't leak memory when previous timings were correct
  - CodingStyle fixes
  - move blank lines around
  
  Regards,
  Steffen
  
  
  Steffen Trumtrar (6):
video: add display_timing and videomode
video: add of helper for videomode
fbmon: add videomode helpers
fbmon: add of_videomode helpers
drm_modes: add videomode helpers
drm_modes: add of_videomode helpers
  
   .../devicetree/bindings/video/display-timings.txt  |  107 ++
   drivers/gpu/drm/drm_modes.c|   70 +++
   drivers/video/Kconfig  |   19 ++
   drivers/video/Makefile |4 +
   drivers/video/display_timing.c |   24 +++
   drivers/video/fbmon.c  |   86 
   drivers/video/of_display_timing.c  |  212 
  
   drivers/video/of_videomode.c   |   47 +
   drivers/video/videomode.c  |   45 +
   include/drm/drmP.h |   12 ++
   include/linux/display_timing.h |   69 +++
   include/linux/fb.h |   12 ++
   include/linux/of_display_timings.h |   20 ++
   include/linux/of_videomode.h   |   17 ++
   include/linux/videomode.h  |   40 
   15 files changed, 784 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/video/display-timings.txt
   create mode 100644 drivers/video/display_timing.c
   create mode 100644 drivers/video/of_display_timing.c
   create mode 100644 drivers/video/of_videomode.c
   create mode 100644 drivers/video/videomode.c
   create mode 100644 include/linux/display_timing.h
   create mode 100644 include/linux/of_display_timings.h
   create mode 100644 include/linux/of_videomode.h
   create mode 100644 include/linux/videomode.h
 
 With the one change that I pointed out, the whole series:
 
 Reviewed-by: Thierry Reding thierry.red...@avionic-design.de
 Acked-by: Thierry Reding thierry.red...@avionic-design.de

Also:

Tested-by: Thierry Reding thierry.red...@avionic-design.de

=)


pgpkNWp33aLAK.pgp
Description: PGP signature

Re: [PATCH v8 1/6] video: add display_timing and videomode

[Thierry Reding]

On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
 diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
[...]
 +void display_timings_release(struct display_timings *disp)
 +{
 + if (disp-timings) {
 + unsigned int i;
 +
 + for (i = 0; i  disp-num_timings; i++)
 + kfree(disp-timings[i]);
 + kfree(disp-timings);
 + }
 + kfree(disp);
 +}

I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
be used from modules.

Thierry


pgpBUyLwaWzk6.pgp
Description: PGP signature

Re: [PATCH v8 2/6] video: add of helper for videomode

[Thierry Reding]

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
[...]
 diff --git a/include/linux/of_display_timings.h 
 b/include/linux/of_display_timings.h
[...]
 +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
 +#define __LINUX_OF_DISPLAY_TIMINGS_H
 +
 +#include linux/display_timing.h
 +
 +#define OF_USE_NATIVE_MODE -1
 +
 +struct display_timings *of_get_display_timings(struct device_node *np);
 +int of_display_timings_exists(struct device_node *np);

This either needs to include linux/of.h or a forward declaration of
struct device_node. Otherwise this will fail to compile if the file
where this is included from doesn't pull linux/of.h in explicitly.

 diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
 +#ifndef __LINUX_OF_VIDEOMODE_H
 +#define __LINUX_OF_VIDEOMODE_H
 +
 +#include linux/videomode.h
 +
 +int of_get_videomode(struct device_node *np, struct videomode *vm, int 
 index);

Same here.

Thierry


pgpDu4puXcSWO.pgp
Description: PGP signature

Re: [PATCH v8 1/6] video: add display_timing and videomode

[Thierry Reding]

On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
 On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
  On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
  [...]
   diff --git a/drivers/video/display_timing.c 
   b/drivers/video/display_timing.c
  [...]
   +void display_timings_release(struct display_timings *disp)
   +{
   + if (disp-timings) {
   + unsigned int i;
   +
   + for (i = 0; i  disp-num_timings; i++)
   + kfree(disp-timings[i]);
   + kfree(disp-timings);
   + }
   + kfree(disp);
   +}
  
  I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
  be used from modules.
  
  Thierry
 
 Yes. Just in time. I was just starting to type the send-email command ;-)

Great! In that case don't forget to also look at my other email before
sending. =)

Thierry


pgpwvfZVVkKPO.pgp
Description: PGP signature