linux-kernel - Re: [PATCH v2] [media] mtk-mdp: Fix g_/s

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Jun 2017 12:10:52 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Minghsiu Tsai <minghsiu.tsai@...iatek.com>
Cc:     Hans Verkuil <hans.verkuil@...co.com>, daniel.thompson@...aro.org,
        Rob Herring <robh+dt@...nel.org>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Daniel Kurtz <djkurtz@...omium.org>,
        Pawel Osciak <posciak@...omium.org>,
        Houlong Wei <houlong.wei@...iatek.com>,
        srv_heupstream@...iatek.com,
        Eddie Huang <eddie.huang@...iatek.com>,
        Yingjoe Chen <yingjoe.chen@...iatek.com>,
        Wu-Cheng Li <wuchengli@...gle.com>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-media@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose
 logic

On 06/19/2017 12:03 PM, Minghsiu Tsai wrote:
> Hi, Hans,
> 
> On Fri, 2017-06-16 at 12:42 +0200, Hans Verkuil wrote:
>> On 05/12/17 04:42, Minghsiu Tsai wrote:
>>> From: Daniel Kurtz <djkurtz@...omium.org>
>>>
>>> Experiments show that the:
>>>   (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
>>
>> Please drop this, since this no longer applies to this patch.
>>
> 
> I will remove it next version
> 
> 
>>>   (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
>>
>> Are you really certain about this?
>>
>> For m2m devices the output (i.e. memory to hardware) typically crops from memory
>> and the capture side (hardware to memory) composes into memory.
>>
>> I.e.: for the output side you crop the part of the memory buffer that you want
>> to process and on the capture side you compose the result into a memory buffer:
>> i.e. the memory buffer might be 1920x1080, but you compose the decoder output
>> into a rectangle of 640x480 at offset 128x128 within that buffer (just an example).
>>
>> CAPTURE using crop would be if, before the data is DMAed, the hardware decoder
>> output is cropped. E.g. if the stream fed to the decoder is 1920x1080, but you
>> want to only DMA a subselection of that, then that would be cropping, and it
>> would go to a memory buffer of the size of the crop selection.
>>
>> OUTPUT using compose is highly unlikely: that means that the frame you give
>> is composed in a larger internal buffer with generated border data around it.
>> Very rare and really only something that a compositor of some sort would do.
>>
> 
> That's strange. In v4l2-ioctl.c, v4l_g_crop()
> OUTPUT is using COMPOSE
> CAPTURE is using CROP

The old crop API can't be used with most (all?) codec drivers. Codec drivers do
exactly the opposite of what the old crop API did.

It is the reason the selection API was created.

The old crop API was written for SDTV receivers where the hardware would crop
the received video. For output (rarely used) the hardware would compose the
image into a larger (PAL or NTSC sizes) frame.

Applications have to use the selection API to work with codec drivers. Just
ignore the old crop ioctls, they are not suitable for such hardware.

> 
> static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> 				struct file *file, void *fh, void *arg)
> ...
> 	/* crop means compose for output devices */
> 	if (V4L2_TYPE_IS_OUTPUT(p->type))
> 		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> 	else
> 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> 
> 	ret = ops->vidioc_g_selection(file, fh, &s);
> 
> 
>> What exactly does the hardware do? Both for the encoder and for the decoder
>> case. Perhaps if I knew exactly what that is, then I can advise.
>>
> 
> NV12M/YUV420M/MT21 -> MDP -> NV12M/YUV420M
> 
> The usage would be like this:
> For decoder:
> decoder -> MT21 -> MDP -> NV12M/YUV420M
> 
> For encoder:
> NV12M/YUV420M -> MDP -> NV12M/YUV420M -> encoder

That doesn't help. Where in these two chains does the cropping (encoder) or
composing (decoder) take place? I assume it is the DMA engine that read from
a rectangle in the source buffer (encoder) or writes to a rectangle in the
destination buffer (decoder).

And in that case the current driver is correct.

I wonder if the g/s_crop description in the V4L2 Specification should get a
big fat warning that it doesn't apply to codec drivers and that the selection
API should be used instead. A patch for that is welcome!

Regards,

	Hans