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

[Laurent Pinchart]

Hi Magnus,

On Wednesday 07 Sep 2016 17:01:06 Magnus Damm wrote:
> 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@vger.kernel.org/msg06589.h
> > tml
> > 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 +-
> >  

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

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Aug 19, 2016 at 10:39 AM, Laurent Pinchart
 wrote:
> 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

Dropped from renesas-drivers, as this branch is based on a very old tree
(v4.8-rc2), and many (but not all!) commits have found their way upstream.

Gr{oetje,eeting}s,

Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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 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@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 +-
>  

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

[Laurent Pinchart]

Hello,

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

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

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

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

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

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

Patches 1/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.

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.

[1] https://www.mail-archive.com/linux-renesas-soc@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 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  4 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  4 +-
 drivers/gpu/drm/omapdrm/omap_plane.c|  4 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 74 +++--
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h   |  2 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  4 +-
 drivers/gpu/drm/tegra/dc.c  | 17 --
 drivers/gpu/drm/vc4/vc4_plane.c |  2 -
 drivers/media/platform/rcar-fcp.c   | 17 +++---
 drivers/media/platform/vsp1/vsp1_drm.c  | 24 
 include/drm/drm_modeset_helper_vtables.h|  4 +-
 include/media/rcar-fcp.h|  5 ++
 include/media/vsp1.h|  3 +
 19 files changed, 126 insertions(+), 82 deletions(-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line