Re: [PATCH v3 10/10] media: aspeed: add a workaround to fix a silicon bug

[Eddie James <eajames@xxxxxxxxxxxxxxxxxx>]

On 6/12/19 2:17 AM, Hans Verkuil wrote:
> Eddie: ping!
>
> Regards,
>
> 	Hans
>
> On 6/6/19 2:53 AM, Jae Hyun Yoo wrote:
> > Hi Eddie,
> >
> > All patches in this series were queued to linux/media tree except this
> > one. Can you please review this patch?
> >
> > Thanks,
> > Jae
> >
> > On 5/31/2019 3:15 PM, Jae Hyun Yoo wrote:
> > > AST2500 silicon revision A1 and A2 have a silicon bug which causes
> > > extremly long capturing time on specific resolutions (1680 width).
> > > To fix the bug, this commit adjusts the capturing window register
> > > setting to 1728 if detected width is 1680. The compression window
> > > register setting will be kept as the original width so output
> > > result will be the same.


Sorry, missed your followup email Jae and assumed everything was merged.


Looks fine to me.

Reviewed-by: Eddie James <eajames@xxxxxxxxxxxxx>


> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> > > ---
> > > v2 -> v3:
> > >    Added more detail comments why the value 1728 is picked.
> > >
> > > v1 -> v2:
> > >    New.
> > >
> > >    drivers/media/platform/aspeed-video.c | 28 ++++++++++++++++++++-------
> > >    1 file changed, 21 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> > > index ba093096a5a7..f899ac3b4a61 100644
> > > --- a/drivers/media/platform/aspeed-video.c
> > > +++ b/drivers/media/platform/aspeed-video.c
> > > @@ -824,8 +824,29 @@ static void aspeed_video_set_resolution(struct aspeed_video *video)
> > >    	struct v4l2_bt_timings *act = &video->active_timings;
> > >    	unsigned int size = act->width * act->height;
> > >    
> > > +	/* Set capture/compression frame sizes */
> > >    	aspeed_video_calc_compressed_size(video, size);
> > >    
> > > +	if (video->active_timings.width == 1680) {
> > > +		/*
> > > +		 * This is a workaround to fix a silicon bug on A1 and A2
> > > +		 * revisions. Since it doesn't break capturing operation of
> > > +		 * other revisions, use it for all revisions without checking
> > > +		 * the revision ID. It picked 1728 which is a very next
> > > +		 * 64-pixels aligned value to 1680 to minimize memory bandwidth
> > > +		 * and to get better access speed from video engine.
> > > +		 */
> > > +		aspeed_video_write(video, VE_CAP_WINDOW,
> > > +				   1728 << 16 | act->height);
> > > +		size += (1728 - 1680) * video->active_timings.height;
> > > +	} else {
> > > +		aspeed_video_write(video, VE_CAP_WINDOW,
> > > +				   act->width << 16 | act->height);
> > > +	}
> > > +	aspeed_video_write(video, VE_COMP_WINDOW,
> > > +			   act->width << 16 | act->height);
> > > +	aspeed_video_write(video, VE_SRC_SCANLINE_OFFSET, act->width * 4);
> > > +
> > >    	/* Don't use direct mode below 1024 x 768 (irqs don't fire) */
> > >    	if (size < DIRECT_FETCH_THRESHOLD) {
> > >    		aspeed_video_write(video, VE_TGS_0,
> > > @@ -842,13 +863,6 @@ static void aspeed_video_set_resolution(struct aspeed_video *video)
> > >    		aspeed_video_update(video, VE_CTRL, 0, VE_CTRL_DIRECT_FETCH);
> > >    	}
> > >    
> > > -	/* Set capture/compression frame sizes */
> > > -	aspeed_video_write(video, VE_CAP_WINDOW,
> > > -			   act->width << 16 | act->height);
> > > -	aspeed_video_write(video, VE_COMP_WINDOW,
> > > -			   act->width << 16 | act->height);
> > > -	aspeed_video_write(video, VE_SRC_SCANLINE_OFFSET, act->width * 4);
> > > -
> > >    	size *= 4;
> > >    
> > >    	if (size != video->srcs[0].size) {