From: Helen Koike <helen.koike@collabora.com>
To: Ezequiel Garcia <ezequiel@collabora.com>,
Tomasz Figa <tfiga@chromium.org>,
Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
kernel@collabora.com,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>,
Philipp Zabel <p.zabel@pengutronix.de>,
Boris Brezillon <boris.brezillon@collabora.com>,
Chris Healy <cphealy@gmail.com>
Subject: Re: [PATCH 0/4] Enable Hantro G1 post-processor
Date: Mon, 16 Sep 2019 15:18:53 -0300 [thread overview]
Message-ID: <d7154e5c-6acc-7ee9-a26b-3f9b4efdbbe0@collabora.com> (raw)
In-Reply-To: <9d7cb0caf39aa4704e5d83fab56a65b305c5b924.camel@collabora.com>
Hello,
On 9/12/19 8:35 AM, Ezequiel Garcia wrote:
> On Thu, 2019-09-12 at 14:52 +0900, Tomasz Figa wrote:
>> On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>> Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit :
>>>> On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote:
>>>>> Hi Ezequiel,
>>>>>
>>>>> On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This series enables the post-processor support available
>>>>>> on the Hantro G1 VPU. The post-processor block can be
>>>>>> pipelined with the decoder hardware, allowing to perform
>>>>>> operations such as color conversion, scaling, rotation,
>>>>>> cropping, among others.
>>>>>>
>>>>>> The decoder hardware needs its own set of NV12 buffers
>>>>>> (the native decoder format), and the post-processor is the
>>>>>> owner of the CAPTURE buffers. This allows the application
>>>>>> get processed (scaled, converted, etc) buffers, completely
>>>>>> transparently.
>>>>>>
>>>>>> This feature is implemented by exposing other CAPTURE pixel
>>>>>> formats to the application (ENUM_FMT). When the application
>>>>>> sets a pixel format other than NV12, the driver will enable
>>>>>> and use the post-processor transparently.
>>>>>
>>>>> I'll try to review the series a bit later, but a general comment here
>>>>> is that the userspace wouldn't have a way to distinguish between the
>>>>> native and post-processed formats. I'm pretty much sure that
>>>>> post-processing at least imposes some power penalty, so it would be
>>>>> good if the userspace could avoid it if unnecessary.
>>>>>
>>>>
>>>> Hm, that's true, good catch.
>>>>
>>>> So, it would be desirable to retain the current behavior of allowing
>>>> the application to just set a different pixel format and get
>>>> a post-processed frame, transparently.
>>>>
>>>> But at the same time, it would be nice if the application is somehow
>>>> aware of the post-processing happening. Maybe we can expose a more
>>>> accurate media controller topology, have applications enable
>>>> the post-processing pipeline explicitly.
>>>
>>> How it works on the stateful side is that userspace set the encoding
>>> type (the codec), then passes a header (in our case, there will be
>>> parsed structures replacing this) first. The driver then configure
>>> capture format, giving a hint of the "default" or "native" format. This
>>> may or may not be sufficient, but it does work in giving userspace a
>>> hint.
>>
>> The bad side of that is that we can't handle more than 1 native format.
>>
>> For the most backwards-compatible behavior, sorting the results of
>> ENUM_FMT according to format preference would allow the applications
>> that choose the first format returned that works for them to choose
>> the best one.
>>
>> For a further improvement, an ENUM_FMT flag that tells the userspace
>> that a format is preferred could work.
>>
>> That said, modelling the pipeline appropriately using the media
>> controller is the idea I like the most, because it's the most
>> comprehensive solution. That would have to be well specified and
>> documented, though, and sounds like a long term effort.
>>
I was playing with vimc to emulate this topology.
What I would suggest is a topology like this:
Device topology
- entity 1: Decoder (2 pads, 3 links)
type V4L2 subdev subtype Unknown flags 0
pad0: Sink
<- "M2M Video Node":1 [ENABLED]
pad1: Source
-> "M2M Video Node":0 [ENABLED]
-> "PostProc":0 []
- entity 4: PostProc (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
pad0: Sink
<- "Decoder":1 []
pad1: Source
-> "M2M Video Node":0 []
- entity 7: M2M Video Node (2 pads, 3 links)
type Node subtype V4L flags 0
device node name /dev/video0
pad0: Sink
<- "Decoder":1 [ENABLED]
<- "PostProc":1 []
pad1: Source
-> "Decoder":0 [ENABLED]
Dot file:
---------
digraph board {
rankdir=TB
n00000001 [label="{{<port0> 0} | Decoder\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
n00000001:port1 -> n00000007
n00000001:port1 -> n00000004:port0 [style=dashed]
n00000004 [label="{{<port0> 0} | PostProc\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
n00000004:port1 -> n00000007 [style=dashed]
n00000007 [label="M2M Video Node\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
n00000007 -> n00000001:port0
}
Also, as you mentioned in patch 4:
> On the YUV-to-RGB path, the post-processing pipeline
> allows to modify the image contrast, brigthness and saturation,
> so additional user controls are added to expose them.
So I think it would make sense to expose these controls in the post-processor subdev,
through a /dev/v4l-subdev0, this avoids having a dynamic set of controls in the video node
depending on the path.
You don't even need to implement VIDIOC_{G,S}_FORMAT in the pads (at least v4l2-compliance
doesn't complain, it is happy with "Not Supported"). So the post-processor /dev/v4l-subdev0
devnode could be used to expose only these controls, and you don't need to expose a devnode
to the Decoder entity above (unless you have another reason).
I hope this helps,
Helen
prev parent reply other threads:[~2019-09-16 18:19 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia
2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia
2019-09-04 10:50 ` Philipp Zabel
2019-09-03 18:17 ` [PATCH 2/4] media: hantro: Simplify buffer helpers Ezequiel Garcia
2019-09-04 10:50 ` Philipp Zabel
2019-09-03 18:17 ` [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation Ezequiel Garcia
2019-09-04 10:17 ` Philipp Zabel
2019-09-04 12:50 ` Ezequiel Garcia
2019-09-03 18:17 ` [PATCH 4/4] media: hantro: Support color conversion via post-processing Ezequiel Garcia
2019-09-09 11:03 ` Hans Verkuil
2019-09-11 8:34 ` Ezequiel Garcia
2019-09-09 7:07 ` [PATCH 0/4] Enable Hantro G1 post-processor Tomasz Figa
2019-09-11 8:27 ` Ezequiel Garcia
2019-09-11 19:48 ` Nicolas Dufresne
2019-09-11 19:49 ` Nicolas Dufresne
2019-09-12 5:52 ` Tomasz Figa
2019-09-12 11:35 ` Ezequiel Garcia
2019-09-16 18:18 ` Helen Koike [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d7154e5c-6acc-7ee9-a26b-3f9b4efdbbe0@collabora.com \
--to=helen.koike@collabora.com \
--cc=boris.brezillon@collabora.com \
--cc=cphealy@gmail.com \
--cc=ezequiel@collabora.com \
--cc=heiko@sntech.de \
--cc=jonas@kwiboo.se \
--cc=kernel@collabora.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=nicolas@ndufresne.ca \
--cc=p.zabel@pengutronix.de \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).