| Message ID | 20190531221548.14757-11-jae.hyun.yoo@linux.intel.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Improve stability and add bug fixes of Aspeed video engine driver | expand |
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. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > 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) { >
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. >> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >> --- >> 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) { >>
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@linux.ibm.com> >>> >>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >>> --- >>> 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) { >>>
On 6/12/2019 8:03 AM, Eddie James wrote: > > 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. Hi Hans, Can you please fix a typo in the commit message when you queue this patch? Thanks in advance! s/extremly/extremely/g > > > Sorry, missed your followup email Jae and assumed everything was merged. > > > Looks fine to me. > > Reviewed-by: Eddie James <eajames@linux.ibm.com> Thanks for your review Eddie! Regards, Jae
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) {
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. Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> --- 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(-)