[PATCH] drm: rcar-du: Fix R-Car Gen3 crash when VSP is disabled

[Magnus Damm]

Hi Geert,

On Wed, Oct 26, 2016 at 4:31 PM, Geert Uytterhoeven
 wrote:
> On Wed, Oct 26, 2016 at 5:31 AM, Magnus Damm  wrote:
>> From: Magnus Damm <damm+renesas at opensource.se>
>>
>> For the DU to operate on R-Car Gen3 hardware a combination of DU
>> and VSP devices are required. Since the DU driver also supports
>> earlier generations hardware the VSP portion is enabled via Kconfig.
>>
>> The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled
>> as a module, however this is not enough to support R-Car Gen3. In
>> the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes
>> when loading the module. This patch is fixing that particular case.
>>
>> In more detail, the crash triggers in drm_atomic_get_plane_state()
>> when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
>>
>> This patch corrects this issue by failing to load the DU driver on
>> R-Car Gen3 when VSP is not available.
>>
>> Signed-off-by: Magnus Damm <damm+renesas at opensource.se>
>> ---
>>
>>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- 0001/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
>> +++ work/drivers/gpu/drm/rcar-du/rcar_du_vsp.h  2016-10-26 
>> 00:01:12.920607110 +0900
>> @@ -70,7 +70,7 @@ void rcar_du_vsp_disable(struct rcar_du_
>>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
>>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
>>  #else
>> -static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
>> +static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 
>> -ENXIO; };
>
> -ENODEV sounds more appropriate

Ok, however -ENXIO is the same error code as the DU driver currently
returns when dealing with the VSP and not finding DT nodes.

To avoid dealing with this again I would prefer skipping per-driver
Kconfig options entirely, for instance something like this:

[PATCH/RFC] Simplify Gen3 DU and VSP Kconfig bits
https://www.spinics.net/lists/linux-sh/msg45978.html

>>  static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
>>  static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
>>  static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
>
> Alternatively,  DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795
> or ARCH_R8A7796 is enabled.

The DU driver has symbol dependencies on VSP and FCP driver code
(V4L2), so we need to take the state of driver support for those
modules into consideration as well instead of only depending on
SoC-specific Kconfig symbols.

On Wed, Oct 26, 2016 at 4:31 PM, Geert Uytterhoeven
 wrote:
>On Wed, Oct 26, 2016 at 5:31 AM, Magnus Damm  wrote:
>> The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled
>> as a module, however this is not enough to support R-Car Gen3. In
>
> Nope, arm64 defconfig on v4.9-rc1:
> # CONFIG_DRM_RCAR_DU is not set

Right, you are correct. Sorry for the noise. It seems that I was using
renesas-drivers-2016-10-18-v4.9-rc1 - not mainline v4.9-rc1.

Without this fix the driver still crashes, but not due to defconfig in v4.9-rc1.

Thanks,

/ magnus

[PATCH] drm: rcar-du: Fix R-Car Gen3 crash when VSP is disabled

[Magnus Damm]

From: Magnus Damm <damm+rene...@opensource.se>

For the DU to operate on R-Car Gen3 hardware a combination of DU
and VSP devices are required. Since the DU driver also supports
earlier generations hardware the VSP portion is enabled via Kconfig.

The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled
as a module, however this is not enough to support R-Car Gen3. In
the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes
when loading the module. This patch is fixing that particular case.

In more detail, the crash triggers in drm_atomic_get_plane_state()
when __drm_atomic_helper_set_config() passes NULL as crtc->primary.

This patch corrects this issue by failing to load the DU driver on
R-Car Gen3 when VSP is not available.

Signed-off-by: Magnus Damm <damm+renesas at opensource.se>
---

 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
+++ work/drivers/gpu/drm/rcar-du/rcar_du_vsp.h  2016-10-26 00:01:12.920607110 
+0900
@@ -70,7 +70,7 @@ void rcar_du_vsp_disable(struct rcar_du_
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
 #else
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; };
 static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };

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

[Magnus Damm]

Hi Laurent,

Thanks for your help with this. Good to see that the DU driver is
getting closer to work with the IPMMU hardware! Please see below for
some feedback from me.

On Fri, Aug 19, 2016 at 5:39 PM, Laurent Pinchart
 wrote:
> Hello,
>
> This patch series fixes the rcar-du-drm driver to support VSP plane sources
> with an IOMMU. It is available for convenience at
>
> git://linuxtv.org/pinchartl/media.git iommu/devel/du
>
> On R-Car Gen3 the DU has no direct memory access but sources planes through
> VSP instances. When an IOMMU is inserted between the VSP and memory, the DU
> framebuffers need to be DMA mapped using the VSP device, not the DU device as
> currently done. The same situation can also be reproduced on Gen2 hardware by
> linking the VSP to the DU in DT [1], effectively disabling direct memory
> access by the DU.
>
> The situation is made quite complex by the fact that different planes can be
> connected to different DU instances, and thus served by different IOMMUs (or,
> in practice on existing hardware, by the same IOMMU but through different
> micro-TLBs). We thus can't allocate and map buffers to the right device in a
> single dma_alloc_wc() operation as done in the DRM CMA GEM helpers.
>
> However, on such setups, the DU DT node doesn't reference IOMMUs as the DU
> does not perform any direct memory access. We can thus keep the GEM object
> allocation unchanged, and the DMA addresses that we receive in the DU driver
> will be physical addresses. Those buffers then need to be mapped to the VSP
> device when they are associated with planes. Fortunately the atomic framework
> provides two plane helper operations, .prepare_fb() and .cleanup_fb() that we
> can use for this purpose.
>
> The reality is slightly more complex than this on Gen3, as an FCP device
> instance sits between VSP instances and memory. It is the FCP devices that are
> connected to the IOMMUs, and buffer mapping thus need to be performed using
> the FCP devices. This isn't required on Gen2 as the platforms don't have any
> FCPs.
>
> Patches 1/6 and 2/6 unconstify the state argument to the .prepare_fb() and
> .cleanup_fb() operations, to allow storing the mapped buffer addresses in the
> state. Patches 3/6 and 4/6 then extend the rcar-fcp driver API to expose the
> FCP struct device. Patch 5/6 extends the vsp1 driver API to allow mapping a
> scatter-gather list to the VSP, with the implementation using the FCP devices
> instead when available. Patch 6/6 then use the vsp1 mapping API in the
> rcar-du-drm driver to map and unmap buffers when needed.
>
> The series has been tested on Gen2 (Lager) only as the Gen3 IOMMU is known to
> be broken.

Slight clarification, the R-Car Gen3 family as a whole does not have
broken IPMMU hardware. Early R-Car H3 revisions do require some errata
handling though, but M3-W and later ES versions and MP of H3 will be
fine. Given the early R-Car H3 errata I agree it makes sense to
develop and test this series on R-Car Gen2 though.

> A possible improvement is to modify the GEM object allocation mechanism to use
> non-contiguous memory when the DU driver detects that all the VSP instances it
> is connected to use an IOMMU (possibly through FCP devices).
>
> An issue has been noticed with synchronization between page flip and VSP
> operation. Buffers get unmapped (and possibly freed) before the VSP is done
> reading them. The problem isn't new, but is much more noticeable with IOMMU
> support enabled as any hardware access to unmapped memory generates an IOMMU
> page fault immediately.
>
> The series unfortunately contain a dependency between DRM and V4L2 patches,
> complicating upstream merge. As there's no urgency to merge patch 6/6 due to
> the IOMMU being broken on Gen3 at the moment, I propose merging patches
> 1/6-2/6 and 3/6-5/6 independently for the next kernel release.
>
> I would particularly appreciate feedback on the APIs introduced by patches 4/6
> and 5/6.

The code in general looks fine to me. The APIs introduced by patches
4/6 and 5/6 seem quite straightforward. Is there something I can do to
help with those?

> [1] https://www.mail-archive.com/linux-renesas-soc at 
> vger.kernel.org/msg06589.html
>
> Laurent Pinchart (6):
>   drm: Don't implement empty prepare_fb()/cleanup_fb()
>   drm: Unconstify state argument to prepare_fb()/cleanup_fb()
>   v4l: rcar-fcp: Don't get/put module reference
>   v4l: rcar-fcp: Add an API to retrieve the FCP device
>   v4l: vsp1: Add API to map and unmap DRM buffers through the VSP
>   drm: rcar-du: Map memory through the VSP device
>
>  drivers/gpu/drm/arc/arcpgu_crtc.c   |  2 -
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  4 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 15 -
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 15 -
>  drivers/gpu/drm/i915/intel_display.c|  4 +-
>  drivers/gpu/drm/i915/intel_drv.h|  4 +-
>  

[RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture

[Magnus Damm]

Hi Marek,

On Fri, Feb 19, 2016 at 5:22 PM, Marek Szyprowski
 wrote:
> This patch replaces ARM-specific IOMMU-based DMA-mapping implementation
> with generic IOMMU DMA-mapping code shared with ARM64 architecture. The
> side-effect of this change is a switch from bitmap-based IO address space
> management to tree-based code. There should be no functional changes
> for drivers, which rely on initialization from generic arch_setup_dna_ops()
> interface. Code, which used old arm_iommu_* functions must be updated to
> new interface.
>
> Signed-off-by: Marek Szyprowski 
> ---

Thanks for your efforts and my apologies for late comments. Just FYI
I'll try your patch (and this series) with the ipmmu-vmsa.c driver on
32-bit ARM and see how it goes. Nice not to have to support multiple
interfaces depending on architecture!

One question that comes to mind is how to handle features.

For instance, the 32-bit ARM code supports DMA_ATTR_FORCE_CONTIGUOUS
while the shared code in drivers/iommu/dma-iommu.c does not. I assume
existing users may rely on such features so from my point of view it
probably makes sense to carry over features from the 32-bit ARM code
into the shared code before pulling the plug.

I also wonder if it is possible to do a step-by-step migration and
support both old and new interfaces in the same binary? That may make
things easier for multiplatform enablement. So far I've managed to
make one IOMMU driver support both 32-bit ARM and 64-bit ARM with some
ugly magic, so adjusting 32-bit ARM dma-mapping code to coexist with
the shared code in drivers/iommu/dma-iommu.c may also be possible. And
probably involving even more ugly magic. =)

Cheers,

/ magnus

[PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 3:48 PM, Wolfram Sang  wrote:
>
>> > This has been tackled as well now. The clock for the GPIO controller was
>> > off, so no interrupt was passed through.
>>
>> Thanks a lot for your efforts! When you have time, can you please show
>> me the patch that fixes that GPIO interrupt problem? I'm asking
>> because I may have ran into a similar issue on Gose or Alt.
>
> For Koelsch, this simply meant I reverted Geert's clocks-off patch. But
> there is a fundamental problem below: GPIO interrupts described with the
> "interrupts" property in DT do not get the underlying GPIO requested.
> And if noone else is using the GPIO block, its clock may be turned off.
>
> I have an idea how to fix it, need to check priorities of the other
> tasks, though.

I guess you mean that the GPIO callbacks include Runtime PM handling
however for irq_chip Runtime PM may not be hooked up so the GPIO block
is in such case is not powered on / get clock enabled?

> BTW, the continous EDID read test on your board is currently at >36
> successful reads, a not a single failure :)

Excellent, thank you!

/ magnus

[PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 6:05 AM, Wolfram Sang  wrote:
>
>> Note that I could not yet read EDID with Magnus' Koelsch.
>
> This has been tackled as well now. The clock for the GPIO controller was
> off, so no interrupt was passed through.

Thanks a lot for your efforts! When you have time, can you please show
me the patch that fixes that GPIO interrupt problem? I'm asking
because I may have ran into a similar issue on Gose or Alt.

Cheers,

/ magnus

[PATCH 0/5] R-Car DU: Add support for R8A7793 and R8A7794

[Magnus Damm]

Hi Laurent,

On Fri, Jul 17, 2015 at 6:06 PM, Laurent Pinchart
 wrote:
> Hi Morimoto-san,
>
> Thank you for your quick reply.
>
> On Friday 17 July 2015 09:03:32 Kuninori Morimoto wrote:
>> Hi Laurent, again
>>
>> >> This patch series adds support for the DU found in the R8A7793 and
>> >> R8A7794 SoCs. It mostly consists of DT bindings updates (1/5 and 2/5)
>> >> with a small driver change in 2/5 to support fixed RGB output routing.
>> >> Patches 4/5 and 5/5 then add DU nodes to the r8a7793.dtsi and
>> >> r8a7794.dtsi, with patch 3/5 adding support for the DU0 clock to
>> >> r8a7794.dtsi.
>> >>
>> >> I will enable DU support for the Gose and Alt board in a later step when
>> >> I'll have access to the schematics. I won't be able to test that due to
>> >> lack of hardware though.
>> >>
>> >> The R8A7794 datasheet documents as single DU clock named DU0, while the
>> >> DU section documents DU0 and DU1 channels. I don't know at this point
>> >> whether this is a datasheet mistake or if the DU0 clock drives both
>> >> channels. I've opted for the latter, but I'd appreciate a confirmation.
>> >
>> > I will check it. please wait
>>
>> According to HW team, MSTP page of datasheet seems wrong
>> Maybe you have already tried, but,
>> can you access to MSTPSR1 :: 27bit (for DU1)
>
> I would try if I had access to an Alt board :-)
>
> Magnus, do you have an Alt board I could access remotely ?

Not sure if it is hooked up right now, but I will make sure you can
get access sometime soon.

Cheers,

/ magnus

[PATCH 00/38] Renesas R-Car DU atomic updates support

[Magnus Damm]

Hi Laurent,

On Thu, Feb 26, 2015 at 1:23 AM, Laurent Pinchart
 wrote:
> Hi Magnus,
>
> On Wednesday 25 February 2015 16:43:53 Magnus Damm wrote:
>> On Wed, Feb 25, 2015 at 1:54 PM, Laurent Pinchart wrote:
>> > Hello,
>> >
>> > This patch series implements atomic updates support for the rcar-du
>> > driver.
>> >
>> > The series starts with four core atomic helpers fixes/cleanups (two from
>> > Daniel that I have included here for completeness). It then follows with
>> > two fixes for the adv7511 driver and height fixes for the rcar-du driver.
>> > It finally gets to work by slowly reworking rcar-du until atomic updates
>> > are fully implemented and the transitional helpers gone.
>> >
>> > The patches are based on Dave's 'next' branch and available at
>> >
>> > git://linuxtv.org/pinchartl/fbdev.git drm/next/atomic
>> >
>> > The last patch contains a hardware plane allocator based solely on state
>> > objects, which could be useful as a base implementation should other
>> > drivers experience similar needs.
>>
>> Many thanks for your efforts on this!
>>
>> It looks to me like the adv7511 fixes would be useful as-is
>> independently of the atomic update changes.
>>
>> [PATCH 05/38] drm: adv7511: Fix DDC error interrupt handling
>> [PATCH 06/38] drm: adv7511: Fix nested sleep when reading EDID
>>
>> Is it possible to fast track just the fixes somehow? I believe that
>> several hardware platforms with adv7511 chips would benefit from these
>> fixes.
>
> Do you mean fast track them to v4.0 or v4.1 ? I certainly want to see them in
> v4.1 (hopefully with the rest of the series as well) and I can mark them as
> stable candidates.

The earlier the better IMO, so v4.0 is of course very nice if possible
however v4.1 is also just fine.

Cheers,

/ magnus

[PATCH 00/38] Renesas R-Car DU atomic updates support

[Magnus Damm]

Hi Laurent,

On Wed, Feb 25, 2015 at 1:54 PM, Laurent Pinchart
 wrote:
> Hello,
>
> This patch series implements atomic updates support for the rcar-du driver.
>
> The series starts with four core atomic helpers fixes/cleanups (two from
> Daniel that I have included here for completeness). It then follows with two
> fixes for the adv7511 driver and height fixes for the rcar-du driver. It
> finally gets to work by slowly reworking rcar-du until atomic updates are
> fully implemented and the transitional helpers gone.
>
> The patches are based on Dave's 'next' branch and available at
>
> git://linuxtv.org/pinchartl/fbdev.git drm/next/atomic
>
> The last patch contains a hardware plane allocator based solely on state
> objects, which could be useful as a base implementation should other drivers
> experience similar needs.

Many thanks for your efforts on this!

It looks to me like the adv7511 fixes would be useful as-is
independently of the atomic update changes.

[PATCH 05/38] drm: adv7511: Fix DDC error interrupt handling
[PATCH 06/38] drm: adv7511: Fix nested sleep when reading EDID

Is it possible to fast track just the fixes somehow? I believe that
several hardware platforms with adv7511 chips would benefit from these
fixes.

Best,

/ magnus

[PATCH] drm/cma: Fix 64-bit size_t build warnings

[Magnus Damm]

From: Magnus Damm <damm+rene...@opensource.se>

Fix warnings related to size_t when building for 64-bit architectures:

drivers/gpu/drm/drm_gem_cma_helper.c: In function ‘drm_gem_cma_create’:
drivers/gpu/drm/drm_gem_cma_helper.c:114:4: warning: format ‘%d’ expects 
argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=]
size);
^
drivers/gpu/drm/drm_gem_cma_helper.c: In function ‘drm_gem_cma_describe’:
drivers/gpu/drm/drm_gem_cma_helper.c:393:4: warning: format ‘%d’ expects 
argument of type ‘int’, but argument 8 has type ‘size_t’ [-Wformat=]
off, _obj->paddr, cma_obj->vaddr, obj->size);

Signed-off-by: Magnus Damm <damm+renesas at opensource.se>
---

 drivers/gpu/drm/drm_gem_cma_helper.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 0001/drivers/gpu/drm/drm_gem_cma_helper.c
+++ work/drivers/gpu/drm/drm_gem_cma_helper.c   2015-02-19 06:29:39.155526831 
+0900
@@ -110,7 +110,7 @@ struct drm_gem_cma_object *drm_gem_cma_c
cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
if (!cma_obj->vaddr) {
-   dev_err(drm->dev, "failed to allocate buffer with size %d\n",
+   dev_err(drm->dev, "failed to allocate buffer with size %zu\n",
size);
ret = -ENOMEM;
goto error;
@@ -388,7 +388,7 @@ void drm_gem_cma_describe(struct drm_gem

off = drm_vma_node_start(>vma_node);

-   seq_printf(m, "%2d (%2d) %08llx %pad %p %d",
+   seq_printf(m, "%2d (%2d) %08llx %pad %p %zu",
obj->name, obj->refcount.refcount.counter,
off, _obj->paddr, cma_obj->vaddr, obj->size);

[RFC/PATCH 1/7] drm/rcar-du: Add OF support

[Magnus Damm]

On Wed, Jan 22, 2014 at 12:32 AM, Laurent Pinchart
 wrote:
> Add DT bindings for the R-Car DU with support for core resources
> (memory, IRQ and clocks). Output configuration must still be passed
> through platform data using OF_DEV_AUXDATA.
>
> Signed-off-by: Laurent Pinchart 
> ---
>  .../devicetree/bindings/video/renesas,du.txt   |  49 +++
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 161 
> -
>  2 files changed, 138 insertions(+), 72 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/renesas,du.txt

Hi Laurent,

Thanks for your patches. I have some minor comments related to r8a7790
vs r8a7791, please see below.

> diff --git a/Documentation/devicetree/bindings/video/renesas,du.txt 
> b/Documentation/devicetree/bindings/video/renesas,du.txt
> new file mode 100644
> index 000..6bd947c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/renesas,du.txt
> @@ -0,0 +1,49 @@
> +* Renesas R-Car Display Unit (DU)
> +
> +Required Properties:
> +
> +  - compatible: must be one of the following.
> +- "renesas,du-r8a7779" for R8A7779 (R-Car H1) compatible DU
> +- "renesas,du-r8a7790" for R8A7790 (R-Car H2) compatible DU
> +- "renesas,du-r8a7790" for R8A7791 (R-Car M2) compatible DU

I suppose the last line above should be "renesas,du-r8a7791", right?

> +  - reg: A list of base address and length of each memory resource, one for
> +each entry in the reg-names property.
> +  - reg-names: Name of the memory resources. The DU requires one memory
> +resource for the DU core (named "du") and one memory resource for each
> +LVDS encoder (named "lvds.x" with "x" being the LVDS controller numerical
> +index).
> +
> +  - interrupt-parent: phandle of the parent interrupt controller.
> +  - interrupts: Interrupt specifiers for the DU interrupts.
> +
> +  - clocks: A list of phandles + clock-specifier pairs, one for each entry in
> +the clock-names property.
> +  - clock-names: Name of the clocks. This property is model-dependent.
> +- R8A7779 uses a single functional clock. The clock doesn't need to be
> +  named.
> +- R8A7790 and R8A7790 use one functional clock per channel and one clock
> +  per LVDS encoder. The functional clocks must be named "du.x" with "x"
> +  being the channel numerical index. The LVDS clocks must be named
> +  "lvds.x" with "x" being the LVDS encoder numerical index.

"R8A7790 and R8A7791"...?

Thanks,

/ magnus