# \[V2 5/7\] video: mmp: add pitch info in mmp\_win structure - List: linux-fbdev - 4 messages, 2 authors - Page: https://ratatoskr.run/linux-fbdev/2013/06/8649721/t ## \[V2 5/7\] video: mmp: add pitch info in mmp\_win structure - From: Daniel Drake \ - Date: 2013-06-21 17:15:37 - Message: https://ratatoskr.run/linux-fbdev/2013/06/8649721.md ```text On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou [ref] wrote: > From: Jing Xiang > > 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 > Signed-off-by: Jett.Zhou > --- > 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 - From: jett zhou \ - Date: 2013-06-24 10:34:42 - Message: https://ratatoskr.run/linux-fbdev/2013/06/8649904.md ```text 2013/6/22 Daniel Drake [ref]: > On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou [ref] wrote: >> From: Jing Xiang >> >> 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 >> Signed-off-by: Jett.Zhou >> --- >> 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 - From: Daniel Drake \ - Date: 2013-06-24 16:17:41 - Message: https://ratatoskr.run/linux-fbdev/2013/06/8649952.md ```text On Mon, Jun 24, 2013 at 4:34 AM, jett zhou [ref] 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 - From: jett zhou \ - Date: 2013-06-25 03:10:20 - Message: https://ratatoskr.run/linux-fbdev/2013/06/8650024.md ```text 2013/6/25 Daniel Drake [ref]: > On Mon, Jun 24, 2013 at 4:34 AM, jett zhou [ref] 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 ```