[PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Yoshihiro Kaneko]

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
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;
+		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);
-- 
1.9.1

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Sergei Shtylyov]

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

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Geert Uytterhoeven]

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

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Sergei Shtylyov]

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

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Laurent Pinchart]

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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Damian Hobson-Garcia]

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

Re: [PATCH/RFC] v4l: vsp1: Align crop rectangle to even boundary for YUV formats

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart