Re: [PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi again,

On 2018-05-11 13:10:37 +0200, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your work.
> 
> On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> > The crop_scale routine uses the crop rectangle memebers to configure the
> > VIN clipping rectangle. When crop is not configured all its fields are
> > 0s, and setting the clipping rectangle vertical and horizontal extensions
> > to (0 - 1) causes the registers to be written with an incorrect
> > 0xffffffff value.
> 
> This is an interesting find and a clear bug in my code. But I can't see 
> how crop ever could be 0. When s_fmt is called it always resets the crop 
> and compose width's to the requested format size.
> 
> I'm curious how you found this bug, I tried to reproduce it but could 
> not. 

My bad I was looking at the wrong thing, yes I can reproduce this on 
CSI-2 capture as well. Really nice find!

> This is indeed something we should fix! But I think the proper fix is 
>not allowing crop to be 0 and not treating the symptom in 
>rvin_crop_scale_comp().
> 
> > 
> > Fix this by using the actual format width and height when no crop
> > rectangle has been programmed.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index b41ba9a..ea7a120 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> >  
> >  void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  {
> > -	/* Set Start/End Pixel/Line Pre-Clip */
> > +	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> > +				      vin->format.width;
> > +	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> > +					vin->format.height;
> > +
> > +	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
> >  	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > +	rvin_write(vin, width - 1, VNEPPRC_REG);
> >  
> >  	switch (vin->format.field) {
> >  	case V4L2_FIELD_INTERLACED:
> >  	case V4L2_FIELD_INTERLACED_TB:
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
> >  		break;
> >  	default:
> >  		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height - 1, VNELPRC_REG);
> >  		break;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux