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

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

[Jett.Zhou]

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];
 };
 
 struct mmp_addr {
-- 
1.7.0.4

[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

[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

[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

[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

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