Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-16 Thread Helen Koike
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  
>> 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  
> 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
n0001 [label="{{ 0} | Decoder\n | { 1}}", 
shape=Mrecord, style=filled, fillcolor=green]
n0001:port1 -> n0007
n0001:port1 -> n0004:port0 [style=dashed]
n0004 [label="{{ 0} | PostProc\n | { 1}}", 
shape=Mrecord, style=filled, fillcolor=green]
n0004:port1 -> n0007 [style=dashed]
n0007 [label="M2M Video Node\n/dev/video0", shape=box, 
style=filled, fillcolor=yellow]
n0007 -> n0001: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 th

Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-12 Thread Ezequiel Garcia
On Thu, 2019-09-12 at 14:52 +0900, Tomasz Figa wrote:
> On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne  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  
> > > > 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.
> 

Completely agreed.

Thanks,
Ezequiel



Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-11 Thread Tomasz Figa
On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne  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  
> > > 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.

Best regards,
Tomasz


Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-11 Thread Nicolas Dufresne
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  
> > 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.

> 
> Thanks for the feedback,
> Ezequiel
> 


signature.asc
Description: This is a digitally signed message part


Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-11 Thread Nicolas Dufresne
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  
> > 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.

> 
> Thanks for the feedback,
> Ezequiel
> 


signature.asc
Description: This is a digitally signed message part


Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-11 Thread Ezequiel Garcia
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  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.

Thanks for the feedback,
Ezequiel



Re: [PATCH 0/4] Enable Hantro G1 post-processor

2019-09-09 Thread Tomasz Figa
Hi Ezequiel,

On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia  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.

Best regards,
Tomasz