linux-kernel - Re: [PATCH v3 2/3] media: meson: add v4l2 m2m video decoder driver

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]

Message-ID: <10d8641a-f605-5693-676c-02ca023259da@xs4all.nl>
Date:   Mon, 1 Oct 2018 14:09:28 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Maxime Jourdan <mjourdan@...libre.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH v3 2/3] media: meson: add v4l2 m2m video decoder driver

On 10/01/2018 01:57 PM, Maxime Jourdan wrote:
> Le lun. 1 oct. 2018 à 12:26, Hans Verkuil <hverkuil@...all.nl> a écrit :
>>
>> On 09/28/2018 04:28 PM, Maxime Jourdan wrote:
>>> +             }
>>> +
>>> +             return 0;
>>> +     }
>>> +
>>> +     switch (q->type) {
>>> +     case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> +             sizes[0] = amvdec_get_output_size(sess);
>>> +             *num_planes = 1;
>>> +             break;
>>> +     case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> +             switch (sess->pixfmt_cap) {
>>> +             case V4L2_PIX_FMT_NV12M:
>>> +                     sizes[0] = output_size;
>>> +                     sizes[1] = output_size / 2;
>>> +                     *num_planes = 2;
>>> +                     break;
>>> +             case V4L2_PIX_FMT_YUV420M:
>>> +                     sizes[0] = output_size;
>>> +                     sizes[1] = output_size / 4;
>>> +                     sizes[2] = output_size / 4;
>>> +                     *num_planes = 3;
>>> +                     break;
>>> +             default:
>>> +                     return -EINVAL;
>>> +             }
>>> +             *num_buffers = min(max(*num_buffers, fmt_out->min_buffers),
>>> +                                fmt_out->max_buffers);
>>
>> You can use clamp here. That's easier to read.
>>
> 
> Ack
> 
>>> +             /* The HW needs all buffers to be configured during startup */
>>
>> Why? I kind of expected to see 'q->min_buffers_needed = fmt_out->min_buffers'
>> here. I think some more information is needed here in the comment.
>>
> 
> I'll extend the comments to reflect the following:
> 
> All codecs in the Amlogic vdec need the full available buffer list to
> be configured at startup, i.e all buffer phy addrs must be written to
> registers prior to decoding.
> The firmwares then decide how they use those buffers and the
> interrupts only tell me "the decoder has written a frame to buffer N°
> X".
> 
> fmt_out->min_buffers and fmt_out->max_buffers refer to the min/max
> amount of buffers that can be setup during initialization. In the case
> of MPEG2, the firmware expects 8 buffers, no more no less, so both
> min_buffers and max_buffers have the value "8".
> 
> But even if those values differ (as for H.264 that will come later),
> the firmware still expects all allocated buffers to be setup in
> registers. As such, q->min_buffers_needed must reflect the total
> amount of CAPTURE buffers.

That's much better, thank you! Now I understand why you do this :-)

Regards,

	Hans