by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH v2 08/10] dt-bindings: media: Document bindings for the Sunxi-Cedrus VPU driver

Hi Paul, Philipp,

On Fri, Apr 20, 2018 at 1:04 AM Philipp Zabel <[email protected]>
wrote:

> Hi Paul,

> On Thu, 2018-04-19 at 17:45 +0200, Paul Kocialkowski wrote:
> > This adds a device-tree binding document that specifies the properties
> > used by the Sunxi-Cedurs VPU driver, as well as examples.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > .../devicetree/bindings/media/sunxi-cedrus.txt | 50
++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> > create mode 100644
Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/sunxi-cedrus.txt
b/Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> > new file mode 100644
> > index 000000000000..71ad3f9c3352
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> > @@ -0,0 +1,50 @@
> > +Device-tree bindings for the VPU found in Allwinner SoCs, referred to
as the
> > +Video Engine (VE) in Allwinner literature.
> > +
> > +The VPU can only access the first 256 MiB of DRAM, that are DMA-mapped
starting
> > +from the DRAM base. This requires specific memory allocation and
handling.

And no IOMMU? Brings back memories.

> > +
> > +Required properties:
> > +- compatible : "allwinner,sun4i-a10-video-engine";
> > +- memory-region : DMA pool for buffers allocation;
> > +- clocks : list of clock specifiers, corresponding to
entries in
> > + the clock-names property;
> > +- clock-names : should contain "ahb", "mod" and "ram"
entries;
> > +- assigned-clocks : list of clocks assigned to the VE;
> > +- assigned-clocks-rates : list of clock rates for the clocks assigned
to the VE;
> > +- resets : phandle for reset;
> > +- interrupts : should contain VE interrupt number;
> > +- reg : should contain register base and length
of VE.
> > +
> > +Example:
> > +
> > +reserved-memory {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > +
> > + /* Address must be kept in the lower 256 MiBs of DRAM for VE. */
> > + ve_memory: cma@4a000000 {
> > + compatible = "shared-dma-pool";
> > + reg = <0x4a000000 0x6000000>;
> > + no-map;
> > + linux,cma-default;
> > + };
> > +};
> > +
> > +video-engine@1c0e000 {

> This is not really required by any specification, and not as common as
> gpu@..., but could this reasonably be called "vpu@1c0e000" to follow
> somewhat-common practice?

AFAIR the name is supposed to be somewhat readable for someone that doesn't
know the hardware. To me, "video-engine" sounds more obvious than "vpu",
but we actually use "codec" already, in case of MFC and JPEG codec on
Exynos. If encode/decode is the only functionality of this block, I'd
personally go with "codec". If it can do other things, e.g.
scaling/rotation without encode/decode, I'd probably call it
"video-processor".

Best regards,
Tomasz

2018-04-20 07:24:56

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 01/10] media: v4l2-ctrls: Add missing v4l2 ctrl unlock

On Thu, Apr 19, 2018 at 05:41:15PM +0200, Paul Kocialkowski wrote:
> This adds a missing v4l2_ctrl_unlock call that is required to avoid
> deadlocks.

Maybe you can explain what the deadlock scenario is?

> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index f67e9f5531fa..ba05a8b9a095 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3614,10 +3614,12 @@ void v4l2_ctrl_request_complete(struct media_request *req,
> continue;
>
> v4l2_ctrl_lock(ctrl);
> +
> if (ref->req)
> ptr_to_ptr(ctrl, ref->req->p_req, ref->p_req);
> else
> ptr_to_ptr(ctrl, ctrl->p_cur, ref->p_req);
> +

I'm not sure that this is relevant in this patch.

Maxime

--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (1.06 kB)
signature.asc (849.00 B)
Download all attachments

2018-04-20 07:25:28

On Fri, May 04, 2018 at 03:57:48PM +0200, Paul Kocialkowski wrote:
> On Fri, 2018-05-04 at 15:40 +0200, Maxime Ripard wrote:
> > On Fri, May 04, 2018 at 02:04:38PM +0200, Paul Kocialkowski wrote:
> > > On Fri, 2018-05-04 at 11:15 +0200, Maxime Ripard wrote:
> > > > On Fri, May 04, 2018 at 10:47:44AM +0200, Paul Kocialkowski wrote:
> > > > > > > > > + reg = <0x01c0e000 0x1000>;
> > > > > > > > > + memory-region = <&ve_memory>;
> > > > > > > >
> > > > > > > > Since you made the CMA region the default one, you don't
> > > > > > > > need
> > > > > > > > to
> > > > > > > > tie
> > > > > > > > it to that device in particular (and you can drop it being
> > > > > > > > mandatory
> > > > > > > > from your binding as well).
> > > > > > >
> > > > > > > What if another driver (or the system) claims memory from
> > > > > > > that
> > > > > > > zone
> > > > > > > and
> > > > > > > that the reserved memory ends up not being available for the
> > > > > > > VPU
> > > > > > > anymore?
> > > > > > >
> > > > > > > Acccording to the reserved-memory documentation, the
> > > > > > > reusable
> > > > > > > property
> > > > > > > (that we need for dmabuf) puts a limitation that the device
> > > > > > > driver
> > > > > > > owning the region must be able to reclaim it back.
> > > > > > >
> > > > > > > How does that work out if the CMA region is not tied to a
> > > > > > > driver
> > > > > > > in
> > > > > > > particular?
> > > > > >
> > > > > > I'm not sure to get what you're saying. You have the property
> > > > > > linux,cma-default in your reserved region, so the behaviour
> > > > > > you
> > > > > > described is what you explicitly asked for.
> > > > >
> > > > > My point is that I don't see how the driver can claim back (part
> > > > > of)
> > > > > the
> > > > > reserved area if the area is not explicitly attached to it.
> > > > >
> > > > > Or is that mechanism made in a way that all drivers wishing to
> > > > > use
> > > > > the
> > > > > reserved memory area can claim it back from the system, but
> > > > > there is
> > > > > no
> > > > > priority (other than first-come first-served) for which drivers
> > > > > claims
> > > > > it back in case two want to use the same reserved region (in a
> > > > > scenario
> > > > > where there isn't enough memory to allow both drivers)?
> > > >
> > > > This is indeed what happens. Reusable is to let the system use the
> > > > reserved memory for things like caches that can easily be dropped
> > > > when
> > > > a driver wants to use the memory in that reserved area. Once that
> > > > memory has been allocated, there's no claiming back, unless that
> > > > memory segment was freed of course.
> > >
> > > Thanks for the clarification. So in our case, perhaps the best fit
> > > would
> > > be to make that area the default CMA pool so that we can be ensured
> > > that
> > > the whole 96 MiB is available for the VPU and that no other consumer
> > > of
> > > CMA will use it?
> >
> > The best fit for what use case ? We already discussed this, and I
> > don't see any point in having two separate CMA regions. If you have a
> > reasonably sized region that will accomodate for both the VPU and
> > display engine, why would we want to split them?
>
> The use case I have in mind is boilerplate use of the VPU with the
> display engine, say with DMAbuf.
>
> It wasn't exactly clear in my memory whether we had decided that the CMA
> pool we use for the VPU should also be used for other CMA consumers (I
> realize that this is what we've been doing all along by having
> linux,cma-default; though).
>
> The fact that the memory region will accomodate for both the display
> engine and the VPU is not straightforward IMO and I think this has to be
> an explicit choice that we take. I was under the impression that we
> chose the 96 MiB because that's what the Allwinner reference code does.
> Does the reference code also use this pool for display?

Yes

> I liked the idea of having 96 MiB fully reserved to the VPU because it
> allows us to provide a limit on the use case, such as "this guarantees N
> buffers for resolution foo in format bar". If the display engine also
> uses it, then the limit also depends on how many GEM buffers are
> allocated (unless I'm missing something).

This also guarantees that you take away a fifth of the RAM on some
boards. If we had yet another CMA pool of 64MB as is the multi_v7
defconfig, that's a third of your RAM that's gone, possibly for no
particular reason.

If we make the math, let's say that we are running a system with 4
planes used in 1080p, with 4 Bpp, in double buffering (which is
already an unlikely setup). Let's add on top of that that we're
decoding a 1080p video with 8 buffers pre-allocated with 2Bpp (in
YUV422). Which really seems extreme now :)

And we're at 80MB. My guess is that your memory bus is going to be
dead before you need to use all that memory.

Maxime

--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (5.02 kB)
signature.asc (849.00 B)
Download all attachments