| Message ID | 1430327133-8461-1-git-send-email-ykaneko0929@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello. On 4/29/2015 8:05 PM, Yoshihiro Kaneko wrote: > From: Damian Hobson-Garcia <dhobsong@igel.co.jp> > Make sure that there are valid values in the crop rectangle to ensure > that the color plane doesn't get shifted when cropping. > Since there is no distintion between 12bit and 16bit YUV formats in ?istin?tion. > at the subdev level, use the more restrictive 12bit limits for all YUV > formats. > Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > --- > This patch is based on the master branch of linuxtv.org/media_tree.git. > drivers/media/platform/vsp1/vsp1_rwpf.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c > index fa71f46..9fed0b2 100644 > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > @@ -197,11 +197,21 @@ int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, > */ > format = vsp1_entity_get_pad_format(&rwpf->entity, cfg, RWPF_PAD_SINK, > sel->which); > + > + if (format->code == MEDIA_BUS_FMT_AYUV8_1X32) { > + sel->r.left = (sel->r.left + 1) & ~1; > + sel->r.top = (sel->r.top + 1) & ~1; There's ALIGN() macro just for that. > + sel->r.width = (sel->r.width) & ~1; > + sel->r.height = (sel->r.height) & ~1; Parens not needed. > + } > + > sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2); > sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2); > if (rwpf->entity.type == VSP1_ENTITY_WPF) { > - sel->r.left = min_t(unsigned int, sel->r.left, 255); > - sel->r.top = min_t(unsigned int, sel->r.top, 255); > + int maxcrop = > + format->code == MEDIA_BUS_FMT_AYUV8_1X32 ? 254 : 255; I think you need an empty line here. > + sel->r.left = min_t(unsigned int, sel->r.left, maxcrop); > + sel->r.top = min_t(unsigned int, sel->r.top, maxcrop); > } > sel->r.width = min_t(unsigned int, sel->r.width, > format->width - sel->r.left); WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 30, 2015 at 1:22 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: >> Since there is no distintion between 12bit and 16bit YUV formats in > > ?istin?tion. Distinction? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 4/30/2015 2:44 PM, Geert Uytterhoeven wrote: >>> Since there is no distintion between 12bit and 16bit YUV formats in >> ?istin?tion. > Distinction? Sorry, yes. :-) > Gr{oetje,eeting}s, > Geert WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thursday 30 April 2015 14:22:02 Sergei Shtylyov wrote: > On 4/29/2015 8:05 PM, Yoshihiro Kaneko wrote: > > From: Damian Hobson-Garcia <dhobsong@igel.co.jp> > > > > Make sure that there are valid values in the crop rectangle to ensure > > that the color plane doesn't get shifted when cropping. > > Since there is no distintion between 12bit and 16bit YUV formats in > > ?istin?tion. > > > at the subdev level, use the more restrictive 12bit limits for all YUV > > formats. I would like to mention in the commit message that only the top coordinate constraints differ between the YUV formats, as the subsampling coefficient is always two in the horizontal direction. Do you foresee a use case for odd cropping top coordinates ? > > Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > --- > > > > This patch is based on the master branch of linuxtv.org/media_tree.git. > > > > drivers/media/platform/vsp1/vsp1_rwpf.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > > b/drivers/media/platform/vsp1/vsp1_rwpf.c index fa71f46..9fed0b2 100644 > > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > > @@ -197,11 +197,21 @@ int vsp1_rwpf_set_selection(struct v4l2_subdev > > *subdev,> > > */ > > format = vsp1_entity_get_pad_format(&rwpf->entity, cfg, > > RWPF_PAD_SINK, > > sel->which); > > + > > + if (format->code == MEDIA_BUS_FMT_AYUV8_1X32) { > > + sel->r.left = (sel->r.left + 1) & ~1; > > + sel->r.top = (sel->r.top + 1) & ~1; > > There's ALIGN() macro just for that. > > > + sel->r.width = (sel->r.width) & ~1; > > + sel->r.height = (sel->r.height) & ~1; > > Parens not needed. round_down() could also be used, it might be more readable. > > + } > > + > > sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2); > > sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2); If format->width (height) is odd and sel->r.left (top) is larger than format- >width (height) - 2 then the resulting value will become odd. The resulting configuration will not be accepted at streamon time as the video node width and height are correctly rounded based on YUV subsampling and will thus not match the subdev width and height. However, it would be good to enforce the constraint better at the subdev level, by moving the above alignment code after these two lines. > > if (rwpf->entity.type == VSP1_ENTITY_WPF) { > > - sel->r.left = min_t(unsigned int, sel->r.left, 255); > > - sel->r.top = min_t(unsigned int, sel->r.top, 255); > > + int maxcrop = I would declare maxcrop as an unsigned int. > > + format->code == MEDIA_BUS_FMT_AYUV8_1X32 ? 254 : 255; > > I think you need an empty line here. > > > + sel->r.left = min_t(unsigned int, sel->r.left, maxcrop); > > + sel->r.top = min_t(unsigned int, sel->r.top, maxcrop); Is this needed ? Based on what I understand from the datasheet the WPF crops the image before passing it to the DMA engine. At that point YUV data isn't subsampled, so it looks like we don't need to restrict the left and top to even values. > > } > > sel->r.width = min_t(unsigned int, sel->r.width, > > format->width - sel->r.left);
Hello Laurent, On 2015-05-04 7:13 AM, Laurent Pinchart wrote: > Hello, > > On Thursday 30 April 2015 14:22:02 Sergei Shtylyov wrote: >> On 4/29/2015 8:05 PM, Yoshihiro Kaneko wrote: >>> From: Damian Hobson-Garcia <dhobsong@igel.co.jp> >>> >>> Make sure that there are valid values in the crop rectangle to ensure >>> that the color plane doesn't get shifted when cropping. >>> Since there is no distintion between 12bit and 16bit YUV formats in >> >> ?istin?tion. >> >>> at the subdev level, use the more restrictive 12bit limits for all YUV >>> formats. > > I would like to mention in the commit message that only the top coordinate > constraints differ between the YUV formats, as the subsampling coefficient is > always two in the horizontal direction. I believe that the height value has the same constraint as the top. > > Do you foresee a use case for odd cropping top coordinates ? There might be a case when you're blending surfaces together and one extends beyond the boundary of the other and you want to clip away the non-overlapping portions. >>> if (rwpf->entity.type == VSP1_ENTITY_WPF) { >>> - sel->r.left = min_t(unsigned int, sel->r.left, 255); >>> - sel->r.top = min_t(unsigned int, sel->r.top, 255); >>> + int maxcrop = > > I would declare maxcrop as an unsigned int. > >>> + format->code == MEDIA_BUS_FMT_AYUV8_1X32 ? 254 : 255; >> >> I think you need an empty line here. >> >>> + sel->r.left = min_t(unsigned int, sel->r.left, maxcrop); >>> + sel->r.top = min_t(unsigned int, sel->r.top, maxcrop); > > Is this needed ? Based on what I understand from the datasheet the WPF crops > the image before passing it to the DMA engine. At that point YUV data isn't > subsampled, so it looks like we don't need to restrict the left and top to > even values. > I think that you're correct. There is no subsampling at this stage in the pipeline so the maxcrop setting should be fine at 255 regardless of the format. Thank you, Damian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Damian, On Monday 18 May 2015 17:17:52 Damian Hobson-Garcia wrote: > On 2015-05-04 7:13 AM, Laurent Pinchart wrote: > > On Thursday 30 April 2015 14:22:02 Sergei Shtylyov wrote: > >> On 4/29/2015 8:05 PM, Yoshihiro Kaneko wrote: > >>> From: Damian Hobson-Garcia <dhobsong@igel.co.jp> > >>> > >>> Make sure that there are valid values in the crop rectangle to ensure > >>> that the color plane doesn't get shifted when cropping. > >>> Since there is no distintion between 12bit and 16bit YUV formats in > >> > >> ?istin?tion. > >> > >>> at the subdev level, use the more restrictive 12bit limits for all YUV > >>> formats. > > > > I would like to mention in the commit message that only the top coordinate > > constraints differ between the YUV formats, as the subsampling coefficient > > is always two in the horizontal direction. > > I believe that the height value has the same constraint as the top. Sure, I meant only the vertical coordinates, sorry. > > Do you foresee a use case for odd cropping top coordinates ? > > There might be a case when you're blending surfaces together and one > extends beyond the boundary of the other and you want to clip away the > non-overlapping portions. Let's wait until someone requests support for that use case :-) > >>> if (rwpf->entity.type == VSP1_ENTITY_WPF) { > >>> - sel->r.left = min_t(unsigned int, sel->r.left, 255); > >>> - sel->r.top = min_t(unsigned int, sel->r.top, 255); > >>> + int maxcrop = > > > > I would declare maxcrop as an unsigned int. > > > >>> + format->code == MEDIA_BUS_FMT_AYUV8_1X32 ? 254 : 255; > >> > >> I think you need an empty line here. > >> > >>> + sel->r.left = min_t(unsigned int, sel->r.left, maxcrop); > >>> + sel->r.top = min_t(unsigned int, sel->r.top, maxcrop); > > > > Is this needed ? Based on what I understand from the datasheet the WPF > > crops the image before passing it to the DMA engine. At that point YUV > > data isn't subsampled, so it looks like we don't need to restrict the > > left and top to even values. > > I think that you're correct. There is no subsampling at this stage in > the pipeline so the maxcrop setting should be fine at 255 regardless of > the format.
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c index fa71f46..9fed0b2 100644 --- a/drivers/media/platform/vsp1/vsp1_rwpf.c +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c @@ -197,11 +197,21 @@ int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, */ format = vsp1_entity_get_pad_format(&rwpf->entity, cfg, RWPF_PAD_SINK, sel->which); + + if (format->code == MEDIA_BUS_FMT_AYUV8_1X32) { + sel->r.left = (sel->r.left + 1) & ~1; + sel->r.top = (sel->r.top + 1) & ~1; + sel->r.width = (sel->r.width) & ~1; + sel->r.height = (sel->r.height) & ~1; + } + sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2); sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2); if (rwpf->entity.type == VSP1_ENTITY_WPF) { - sel->r.left = min_t(unsigned int, sel->r.left, 255); - sel->r.top = min_t(unsigned int, sel->r.top, 255); + int maxcrop = + format->code == MEDIA_BUS_FMT_AYUV8_1X32 ? 254 : 255; + sel->r.left = min_t(unsigned int, sel->r.left, maxcrop); + sel->r.top = min_t(unsigned int, sel->r.top, maxcrop); } sel->r.width = min_t(unsigned int, sel->r.width, format->width - sel->r.left);