linux-kernel - Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution change event

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: <a6a50052-42cb-e4d7-a387-bea1f852345b@collabora.com>
Date:   Tue, 20 Jun 2017 16:08:39 +0200
From:   Thierry Escande <thierry.escande@...labora.com>
To:     Andrzej Pietrasiewicz <andrzej.p@...sung.com>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution
 change event

Hi Andrzej,

On 20/06/2017 12:51, Andrzej Pietrasiewicz wrote:
> Hi Thierry,
> 
> W dniu 19.06.2017 o 15:50, Thierry Escande pisze:
>> Hi Andrzej,
>>
>> On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:
>>> Hi Thierry,
>>>
>>> Thank you for the patch.
>>>
>>> Can you give a use case for resolution change event?
>> Unfortunately, the original commit does not mention any clear use case.
>> I've asked to the patch author for more information.
> 
> Can you please share what you learn about it if the author gets back to 
> you?
> Now that we don't know why to apply a patch I guess we should not do it.
This event is used in Chromium by the V4L2 jpeg decode accelerator to 
allocate output buffer. Please see:
https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.cc?rcl=91793c6ef94f05e93d258db8c7f3cad59819c6b8&l=585

I'll add a note in the commit message.

> 
> <snip>
> 
>>>> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
>>>> vb2_buffer *vb)
>>>>                return;
>>>>            }
>>>> -        q_data = &ctx->out_q;
>>>> -        q_data->w = tmp.w;
>>>> -        q_data->h = tmp.h;
>>>> -        q_data->sos = tmp.sos;
>>>> -        memcpy(q_data->dht.marker, tmp.dht.marker,
>>>> -               sizeof(tmp.dht.marker));
>>>> -        memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
>>>> -        q_data->dht.n = tmp.dht.n;
>>>> -        memcpy(q_data->dqt.marker, tmp.dqt.marker,
>>>> -               sizeof(tmp.dqt.marker));
>>>> -        memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
>>>> -        q_data->dqt.n = tmp.dqt.n;
>>>> -        q_data->sof = tmp.sof;
>>>> -        q_data->sof_len = tmp.sof_len;
>>>> -
>>>> -        q_data = &ctx->cap_q;
>>>> -        q_data->w = tmp.w;
>>>> -        q_data->h = tmp.h;
>>>
>>>
>>> Why is this part removed?
>> This has not been removed.
>> The &tmp s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
>> then copied field-by-field into ctx->out_q (through q_data pointer).
>> With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
>> avoids the copy.
> 
> It seems that changing field-by-field copying to passing a pointer
> directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
> should be in a separate patch.
Will do.

Regards,
  Thierry