Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

[Jean-Christophe PLAGNIOL-VILLARD]

On 20:29 Sun 09 Jun     , Jett.Zhou wrote:
> From: Jing Xiang <jxiang@marvell.com>
> 

this is impossible to follow

resend the patch series fully with a PATCH 0

as I just go back from vacation I'll not read this series until then

Best Regards,
J.

> Add pitch length info of graphics/video layer for mmp_win, if it is
> YUV format of video layer, u/v pitch will non-zero.
> 
> Signed-off-by: Jing Xiang <jxiang@marvell.com>
> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
> ---
>  include/video/mmp_disp.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
> index b9dd1fb..462e3bd 100644
> --- a/include/video/mmp_disp.h
> +++ b/include/video/mmp_disp.h
> @@ -91,6 +91,11 @@ struct mmp_win {
>  	u16	up_crop;
>  	u16	bottom_crop;
>  	int	pix_fmt;
> +	/*
> +	 * pitch[0]: graphics/video layer line length or y pitch
> +	 * pitch[1]/pitch[2]: video u/v pitch if non-zero
> +	 */
> +	u32	pitch[3];
>  };
>  
>  struct mmp_addr {
> -- 
> 1.7.0.4
> 

Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

[Daniel Drake]

On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou <jtzhou@marvell.com> wrote:
> From: Jing Xiang <jxiang@marvell.com>
>
> Add pitch length info of graphics/video layer for mmp_win, if it is
> YUV format of video layer, u/v pitch will non-zero.
>
> Signed-off-by: Jing Xiang <jxiang@marvell.com>
> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
> ---
>  include/video/mmp_disp.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
> index b9dd1fb..462e3bd 100644
> --- a/include/video/mmp_disp.h
> +++ b/include/video/mmp_disp.h
> @@ -91,6 +91,11 @@ struct mmp_win {
>         u16     up_crop;
>         u16     bottom_crop;
>         int     pix_fmt;
> +       /*
> +        * pitch[0]: graphics/video layer line length or y pitch
> +        * pitch[1]/pitch[2]: video u/v pitch if non-zero
> +        */
> +       u32     pitch[3];
>  };

Thanks for adding a comment here, but the meaning of this field is
still not clear to me.
In what case is pitch[0] line length, and in which case does it refer
to y pitch?

pitch[1] and pitch[2] refer to u/v pitch respectively, if their own
values are non-zero? (or if not, what value does the "if non-zero"
comment refer to?)

I would recommend rolling this patch into the patch that actually
makes use of this new field.

Daniel

Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

[jett zhou]

2013/6/22 Daniel Drake <dsd@laptop.org>:
> On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou <jtzhou@marvell.com> wrote:
>> From: Jing Xiang <jxiang@marvell.com>
>>
>> Add pitch length info of graphics/video layer for mmp_win, if it is
>> YUV format of video layer, u/v pitch will non-zero.
>>
>> Signed-off-by: Jing Xiang <jxiang@marvell.com>
>> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
>> ---
>>  include/video/mmp_disp.h |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
>> index b9dd1fb..462e3bd 100644
>> --- a/include/video/mmp_disp.h
>> +++ b/include/video/mmp_disp.h
>> @@ -91,6 +91,11 @@ struct mmp_win {
>>         u16     up_crop;
>>         u16     bottom_crop;
>>         int     pix_fmt;
>> +       /*
>> +        * pitch[0]: graphics/video layer line length or y pitch
>> +        * pitch[1]/pitch[2]: video u/v pitch if non-zero
>> +        */
>> +       u32     pitch[3];
>>  };
>
> Thanks for adding a comment here, but the meaning of this field is
> still not clear to me.
> In what case is pitch[0] line length, and in which case does it refer
> to y pitch?
>
> pitch[1] and pitch[2] refer to u/v pitch respectively, if their own
> values are non-zero? (or if not, what value does the "if non-zero"
> comment refer to?)
>
> I would recommend rolling this patch into the patch that actually
> makes use of this new field.
>
> Daniel
Hi Daniel
    pitch is used to represent line length in byte, the usage depends
on pix_fmt.
    If the fmt is YUV , the pitch[0] will be Y length, pitch[1] will
be U length, pitch[2] will be V lenth.
    If the fmt is RGB, the picth[0] will be line lenth, and
pitch[1]/pitch[2] will be 0 and not be used.
    You can refer to pixfmt_to_stride func implementation.
    For the patch rolling, do you mean combine the patch5 and patch6
by one patch?
Thanks


--

----------------------------------
Best Regards
Jett Zhou

Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

[Daniel Drake]

On Mon, Jun 24, 2013 at 4:34 AM, jett zhou <jett.zhou@gmail.com> wrote:
>     pitch is used to represent line length in byte, the usage depends
> on pix_fmt.
>     If the fmt is YUV , the pitch[0] will be Y length, pitch[1] will
> be U length, pitch[2] will be V lenth.
>     If the fmt is RGB, the picth[0] will be line lenth, and
> pitch[1]/pitch[2] will be 0 and not be used.

This description is clear, thanks - hopefully you can write it with
such clarity in the comment :)

>     For the patch rolling, do you mean combine the patch5 and patch6
> by one patch?

I view patch 6 as a cleanup (consolidating and removing duplication of
code), so I would leave that one separate. Patch 6 should not interact
with any pitch[] stuff.

Then you can write a followup patch which adds the pitch[] header,
*and* modifies mmpfb_set_par() to write to pitch[], *and* acts upon
pitch[] in dmafetch_set_fmt (patch 7). This way, the pitch variable is
defined, documented, written to, and acted upon all in the same patch,
the meaning will then be very clear.

Thanks
Daniel

Re: [V2 5/7] video: mmp: add pitch info in mmp_win structure

[jett zhou]

2013/6/25 Daniel Drake <dsd@laptop.org>:
> On Mon, Jun 24, 2013 at 4:34 AM, jett zhou <jett.zhou@gmail.com> wrote:
>>     pitch is used to represent line length in byte, the usage depends
>> on pix_fmt.
>>     If the fmt is YUV , the pitch[0] will be Y length, pitch[1] will
>> be U length, pitch[2] will be V lenth.
>>     If the fmt is RGB, the picth[0] will be line lenth, and
>> pitch[1]/pitch[2] will be 0 and not be used.
>
> This description is clear, thanks - hopefully you can write it with
> such clarity in the comment :)
>
>>     For the patch rolling, do you mean combine the patch5 and patch6
>> by one patch?
>
> I view patch 6 as a cleanup (consolidating and removing duplication of
> code), so I would leave that one separate. Patch 6 should not interact
> with any pitch[] stuff.
>
> Then you can write a followup patch which adds the pitch[] header,
> *and* modifies mmpfb_set_par() to write to pitch[], *and* acts upon
> pitch[] in dmafetch_set_fmt (patch 7). This way, the pitch variable is
> defined, documented, written to, and acted upon all in the same patch,
> the meaning will then be very clear.
>
HI Daniel
    Thanks for your comments.
    I will add more detail description on the comments.
    For patch6, I will seperated it. For another patch, I will combine
pitch header and mmpfb_set_par and dmafetch_set_fmt (patch 7) as one
new patch based on patch6.
    Will send for your review later.
Thanks



--

----------------------------------
Best Regards
Jett Zhou