Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Frederic Chen]

Dear Laurent and Sakari,

I appreciate your messages.

On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote:
> On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote:
> > Hi Frederic,
> > 
> > Could you cc the devicetree list, please?
> > 
> > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> > > This patch adds the binding for describing the shared memory
> > > used to exchange configuration and tuning data between the
> > > co-processor and Digital Image Processing (DIP) unit of the
> > > camera ISP system on Mediatek SoCs.
> > > 
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > new file mode 100644
> > > index 0000000..0ded478
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > @@ -0,0 +1,45 @@
> > > +Mediatek DIP Shared Memory binding
> > > +
> > > +This binding describes the shared memory, which serves the purpose of
> > > +describing the shared memory region used to exchange data between Digital
> > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > +
> > > +The co-processor doesn't have the iommu so we need to use the physical
> > > +address to access the shared buffer in the firmware.
> > > +
> > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > +it can use dma address to access the memory region.
> > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > 
> > What kind of purpose is the memory used for? Buffers containing video data,
> > or something else? Could the buffer objects be mapped on the devices
> > based on the need instead?

The memory buffers contain camera 3A tuning data, which are used by the
co-processor and DIP IP. About mapping the buffer based on the need
instead, I’m not sure I understand this point. Do you mean that
allocating and mapping the memory dynamically?

> 
> And could CMA be used when physically contiguous memory is needed ?

DIP driver does not use CMA now, because the first version will be used
by CrOS but CrOS won’t enable CMA.

> 
> > > +
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > > +
> > > +- reg: required for static allocation (see reserved-memory.txt for
> > > +  the detailed usage)
> > > +
> > > +- alloc-range: required for dynamic allocation. The range must
> > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > +  addressing limitation
> > > +
> > > +- size: required for dynamic allocation. The unit is bytes.
> > > +  If you want to enable the full feature of Digital Processing Unit,
> > > +  you need 20 MB at least.
> > > +
> > > +
> > > +Example:
> > > +
> > > +The following example shows the DIP shared memory setup for MT8183.
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +		reserve-memory-isp_smem {
> > > +			compatible = "mediatek,reserve-memory-dip_smem";
> > > +			size = <0 0x1400000>;
> > > +			alignment = <0 0x1000>;
> > > +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> > > +		};
> > > +	};
> 

Sincerely,

Frederic Chen



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Tomasz Figa]

On Tue, Feb 12, 2019 at 6:37 PM Frederic Chen
<frederic.chen@mediatek.com> wrote:
>
> Dear Laurent and Sakari,
>
> I appreciate your messages.
>
> On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote:
> > On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote:
> > > Hi Frederic,
> > >
> > > Could you cc the devicetree list, please?
> > >
> > > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> > > > This patch adds the binding for describing the shared memory
> > > > used to exchange configuration and tuning data between the
> > > > co-processor and Digital Image Processing (DIP) unit of the
> > > > camera ISP system on Mediatek SoCs.
> > > >
> > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > ---
> > > >  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > new file mode 100644
> > > > index 0000000..0ded478
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > @@ -0,0 +1,45 @@
> > > > +Mediatek DIP Shared Memory binding
> > > > +
> > > > +This binding describes the shared memory, which serves the purpose of
> > > > +describing the shared memory region used to exchange data between Digital
> > > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > > +
> > > > +The co-processor doesn't have the iommu so we need to use the physical
> > > > +address to access the shared buffer in the firmware.
> > > > +
> > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > > +it can use dma address to access the memory region.
> > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > >
> > > What kind of purpose is the memory used for? Buffers containing video data,
> > > or something else? Could the buffer objects be mapped on the devices
> > > based on the need instead?
>
> The memory buffers contain camera 3A tuning data, which are used by the
> co-processor and DIP IP. About mapping the buffer based on the need
> instead, I’m not sure I understand this point. Do you mean that
> allocating and mapping the memory dynamically?
>
> >
> > And could CMA be used when physically contiguous memory is needed ?
>
> DIP driver does not use CMA now, because the first version will be used
> by CrOS but CrOS won’t enable CMA.
>

Thanks Frederic for replying. Let me further clarify what's the problem here.

The co-processor is behind a simple MPU (Memory Protection Unit),
which does not have any mapping capabilities, but can only allow or
deny access to particular parts of the physical address space. That
means that we have to either build some scatter gather capability
inside of the firmware or just do with contiguous allocation.

There is also a security aspect here. The MPU can be accessed from
both the co-processor and CPU, but it has a lockdown mode, which makes
it read only until the SoC is reset. If we allocate the memory
dynamically, we need to keep the MPU unlocked, which automatically
grants the firmware access to all the address space.

For security reasons we decided to go with preallocated memory pool,
which lets us pre-program the MPU and lock it down.

Best regards,
Tomasz

> >
> > > > +
> > > > +
> > > > +Required properties:
> > > > +
> > > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > > > +
> > > > +- reg: required for static allocation (see reserved-memory.txt for
> > > > +  the detailed usage)
> > > > +
> > > > +- alloc-range: required for dynamic allocation. The range must
> > > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > > +  addressing limitation
> > > > +
> > > > +- size: required for dynamic allocation. The unit is bytes.
> > > > +  If you want to enable the full feature of Digital Processing Unit,
> > > > +  you need 20 MB at least.
> > > > +
> > > > +
> > > > +Example:
> > > > +
> > > > +The following example shows the DIP shared memory setup for MT8183.
> > > > +
> > > > + reserved-memory {
> > > > +         #address-cells = <2>;
> > > > +         #size-cells = <2>;
> > > > +         ranges;
> > > > +         reserve-memory-isp_smem {
> > > > +                 compatible = "mediatek,reserve-memory-dip_smem";
> > > > +                 size = <0 0x1400000>;
> > > > +                 alignment = <0 0x1000>;
> > > > +                 alloc-ranges = <0 0x40000000 0 0x50000000>;
> > > > +         };
> > > > + };
> >
>
> Sincerely,
>
> Frederic Chen
>
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings

[Frederic Chen]

Dear Laurent and Sakari,


On Sat, 2019-02-09 at 20:20 +0200, Laurent Pinchart wrote:
> Hello Frederic,
> 
> On Sat, Feb 09, 2019 at 05:59:35PM +0200, Sakari Ailus wrote:
> > Hi Frederic,
> > 
> > Thanks for the patchset.
> > 
> > Could you also cc the devicetree list, please?
> > 
> > On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote:
> > > This patch adds the DT binding documentation for the shared memory
> > > between DIP (Digital Image Processing) unit of the camera ISP system
> > > and the co-processor in Mediatek SoCs.
> > > 
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  .../bindings/media/mediatek,dip_smem.txt           | 29 ++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > new file mode 100644
> > > index 0000000..5533721
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > @@ -0,0 +1,29 @@
> > > +Mediatek ISP Shared Memory Device
> > > +
> > > +Mediatek ISP Shared Memory Device is used to manage shared memory
> > > +among CPU, ISP IPs and coprocessor. It is associated with a reserved
> > > +memory region (Please see Documentation\devicetree\bindings\
> > > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and
> > 
> > s/\\/\//g;
> > 
> > > +and provide the context to allocate memory with dma addresses.
> 
> Does this represent a real device (as in IP core) in the SoC ? There
> seems to be no driver associated with the compatible string defined
> herein in this patch series, what is this node used for ?

It does not represent a real device. It is used for creating the
DIP-specific vb2 buffer allocation context (implemented
in /drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c).
The compatible string has been renamed as “mediatek,dip_smem” in this
patch series and I will correct it in this binding document.

> 
> > > +Required properties:
> > > +- compatible: Should be "mediatek,isp_smem"
> > 
> > s/Should/Shall/
> > 
> > > +
> > > +- iommus: should point to the respective IOMMU block with master port
> > 
> > s/should/shall/
> > 
> > > +  as argument. Please set the ports which may be accessed
> > > +  through the common path. You can see
> > > +  Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> > > +  for the detail.
> > > +
> > > +- mediatek,larb: must contain the local arbiters in the current Socs.
> > 
> > Perhaps "SoCs"?
> > 
> > > +  Please set the larb of camsys for Pass 1 and imgsys for DIP, or both
> > > +  if you are using all the camera function. You can see
> > > +  Documentation/devicetree/bindings/memory-controllers/
> > > +  mediatek,smi-larb.txt for the detail.
> > > +
> > > +Example:
> > > +	isp_smem: isp_smem {
> > > +		compatible = "mediatek,isp_smem";
> > > +		mediatek,larb = <&larb5>;
> > > +		iommus = <&iommu M4U_PORT_CAM_IMGI>;
> > > +	};
> 


Sincerely,

Frederic Chen



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel