Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670666 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution change  event Andrzej Pietrasiewicz <andrzej.p@samsung.com> - 2017-06-20 13:00 +0200
    Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution  change event Thierry Escande <thierry.escande@collabora.com> - 2017-06-20 16:10 +0200

#1670666 — Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution change event

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.

> 

<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.

Andrzej

[toc] | [next] | [standalone]

#1670794 — 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web