| Message ID | 20211207115923.13639-1-dafna.hirschfeld@collabora.com |
|---|---|
| State | Accepted |
| Commit | 9a0e3cd50d3967b669f0f0ea79a7054c7877d95b |
| Headers | show |
| Series | [v2] media: rkisp1: fix grey format iommu page faults | expand |
On 17.01.22 13:34, Kieran Bingham wrote: > Hi Dafna, > > Quoting Dafna Hirschfeld (2021-12-07 11:59:23) >> Currently capturing grey format produces page faults >> on both selfpath and mainpath. To support greyscale >> we can capture YUV422 planar format and configure the U, V >> buffers to the dummy buffer. >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> This is v2 of the patch "media: rkisp1: remove support for V4L2_PIX_FMT_GREY" >> In v1 I removed the grey format. In this version it is 'fixed' >> >> .../platform/rockchip/rkisp1/rkisp1-capture.c | 28 ++++++++++++++----- >> 1 file changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> index 768987d5f2dd..8e982dd0c740 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> @@ -249,7 +249,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { >> .fourcc = V4L2_PIX_FMT_GREY, >> .uv_swap = 0, >> .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA, >> - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400, >> + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, >> .mbus = MEDIA_BUS_FMT_YUYV8_2X8, >> }, >> /* rgb */ >> @@ -631,12 +631,26 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> rkisp1_write(cap->rkisp1, >> buff_addr[RKISP1_PLANE_Y], >> cap->config->mi.y_base_ad_init); >> - rkisp1_write(cap->rkisp1, >> - buff_addr[RKISP1_PLANE_CB], >> - cap->config->mi.cb_base_ad_init); >> - rkisp1_write(cap->rkisp1, >> - buff_addr[RKISP1_PLANE_CR], >> - cap->config->mi.cr_base_ad_init); >> + /* >> + * In order to support grey format we capture >> + * YUV422 planar format from the camera and >> + * set the U and V planes to the dummy buffer >> + */ >> + if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { >> + rkisp1_write(cap->rkisp1, >> + cap->buf.dummy.dma_addr, >> + cap->config->mi.cb_base_ad_init); >> + rkisp1_write(cap->rkisp1, >> + cap->buf.dummy.dma_addr, >> + cap->config->mi.cr_base_ad_init); >> + } else { >> + rkisp1_write(cap->rkisp1, >> + buff_addr[RKISP1_PLANE_CB], >> + cap->config->mi.cb_base_ad_init); >> + rkisp1_write(cap->rkisp1, >> + buff_addr[RKISP1_PLANE_CR], >> + cap->config->mi.cr_base_ad_init); >> + } >> } else { > > Looking at this function, I think I would have initialised a local array > of addresses (either to zero, or to the dummy address?) to then set > values when appropriate, and reduce the number of calls to > rkisp1_write() to a single set of three after the processing. > > It might make the function simpler, and more readable, but it's more > effort, and this does look like it will solve the greyscale format issue > as discussed in earlier threads so I'd leave it up to you if you want to > refactor. Hi, Yes, I'll do that. Interestingly I found out that the patch causing the iommu page fault is https://www.spinics.net/lists/linux-media/msg176089.html Before that patch there are no iommu page faults but the video is corrupted. I can't explain how I didn't find it before, I clearly remember testing the grey format. Thanks, Dafna > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > >> /* >> * Use the dummy space allocated by dma_alloc_coherent to >> -- >> 2.17.1 >>
Hi Dafna, On Sun, Jan 23, 2022 at 11:50:26AM +0200, Dafna Hirschfeld wrote: > On 17.01.22 13:34, Kieran Bingham wrote: > > Quoting Dafna Hirschfeld (2021-12-07 11:59:23) > >> Currently capturing grey format produces page faults > >> on both selfpath and mainpath. To support greyscale > >> we can capture YUV422 planar format and configure the U, V > >> buffers to the dummy buffer. > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> --- > >> This is v2 of the patch "media: rkisp1: remove support for V4L2_PIX_FMT_GREY" > >> In v1 I removed the grey format. In this version it is 'fixed' > >> > >> .../platform/rockchip/rkisp1/rkisp1-capture.c | 28 ++++++++++++++----- > >> 1 file changed, 21 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> index 768987d5f2dd..8e982dd0c740 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> @@ -249,7 +249,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { > >> .fourcc = V4L2_PIX_FMT_GREY, > >> .uv_swap = 0, > >> .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA, > >> - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400, > >> + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > >> .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > >> }, > >> /* rgb */ > >> @@ -631,12 +631,26 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > >> rkisp1_write(cap->rkisp1, > >> buff_addr[RKISP1_PLANE_Y], > >> cap->config->mi.y_base_ad_init); > >> - rkisp1_write(cap->rkisp1, > >> - buff_addr[RKISP1_PLANE_CB], > >> - cap->config->mi.cb_base_ad_init); > >> - rkisp1_write(cap->rkisp1, > >> - buff_addr[RKISP1_PLANE_CR], > >> - cap->config->mi.cr_base_ad_init); > >> + /* > >> + * In order to support grey format we capture > >> + * YUV422 planar format from the camera and > >> + * set the U and V planes to the dummy buffer > >> + */ > >> + if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { > >> + rkisp1_write(cap->rkisp1, > >> + cap->buf.dummy.dma_addr, > >> + cap->config->mi.cb_base_ad_init); > >> + rkisp1_write(cap->rkisp1, > >> + cap->buf.dummy.dma_addr, > >> + cap->config->mi.cr_base_ad_init); > >> + } else { > >> + rkisp1_write(cap->rkisp1, > >> + buff_addr[RKISP1_PLANE_CB], > >> + cap->config->mi.cb_base_ad_init); > >> + rkisp1_write(cap->rkisp1, > >> + buff_addr[RKISP1_PLANE_CR], > >> + cap->config->mi.cr_base_ad_init); > >> + } > >> } else { > > > > Looking at this function, I think I would have initialised a local array > > of addresses (either to zero, or to the dummy address?) to then set > > values when appropriate, and reduce the number of calls to > > rkisp1_write() to a single set of three after the processing. > > > > It might make the function simpler, and more readable, but it's more > > effort, and this does look like it will solve the greyscale format issue > > as discussed in earlier threads so I'd leave it up to you if you want to > > refactor. > > Hi, > Yes, I'll do that. > Interestingly I found out that the patch causing the iommu page fault is > > https://www.spinics.net/lists/linux-media/msg176089.html > > Before that patch there are no iommu page faults but the video is corrupted. > > I can't explain how I didn't find it before, I clearly remember testing the grey format. It seems really weird indeed. Are you getting IOMMU faults on both the main and self paths ? > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > >> /* > >> * Use the dummy space allocated by dma_alloc_coherent to
On 23.01.22 16:32, Laurent Pinchart wrote: > Hi Dafna, > > On Sun, Jan 23, 2022 at 11:50:26AM +0200, Dafna Hirschfeld wrote: >> On 17.01.22 13:34, Kieran Bingham wrote: >>> Quoting Dafna Hirschfeld (2021-12-07 11:59:23) >>>> Currently capturing grey format produces page faults >>>> on both selfpath and mainpath. To support greyscale >>>> we can capture YUV422 planar format and configure the U, V >>>> buffers to the dummy buffer. >>>> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> --- >>>> This is v2 of the patch "media: rkisp1: remove support for V4L2_PIX_FMT_GREY" >>>> In v1 I removed the grey format. In this version it is 'fixed' >>>> >>>> .../platform/rockchip/rkisp1/rkisp1-capture.c | 28 ++++++++++++++----- >>>> 1 file changed, 21 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >>>> index 768987d5f2dd..8e982dd0c740 100644 >>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >>>> @@ -249,7 +249,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { >>>> .fourcc = V4L2_PIX_FMT_GREY, >>>> .uv_swap = 0, >>>> .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA, >>>> - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400, >>>> + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, >>>> .mbus = MEDIA_BUS_FMT_YUYV8_2X8, >>>> }, >>>> /* rgb */ >>>> @@ -631,12 +631,26 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >>>> rkisp1_write(cap->rkisp1, >>>> buff_addr[RKISP1_PLANE_Y], >>>> cap->config->mi.y_base_ad_init); >>>> - rkisp1_write(cap->rkisp1, >>>> - buff_addr[RKISP1_PLANE_CB], >>>> - cap->config->mi.cb_base_ad_init); >>>> - rkisp1_write(cap->rkisp1, >>>> - buff_addr[RKISP1_PLANE_CR], >>>> - cap->config->mi.cr_base_ad_init); >>>> + /* >>>> + * In order to support grey format we capture >>>> + * YUV422 planar format from the camera and >>>> + * set the U and V planes to the dummy buffer >>>> + */ >>>> + if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { >>>> + rkisp1_write(cap->rkisp1, >>>> + cap->buf.dummy.dma_addr, >>>> + cap->config->mi.cb_base_ad_init); >>>> + rkisp1_write(cap->rkisp1, >>>> + cap->buf.dummy.dma_addr, >>>> + cap->config->mi.cr_base_ad_init); >>>> + } else { >>>> + rkisp1_write(cap->rkisp1, >>>> + buff_addr[RKISP1_PLANE_CB], >>>> + cap->config->mi.cb_base_ad_init); >>>> + rkisp1_write(cap->rkisp1, >>>> + buff_addr[RKISP1_PLANE_CR], >>>> + cap->config->mi.cr_base_ad_init); >>>> + } >>>> } else { >>> >>> Looking at this function, I think I would have initialised a local array >>> of addresses (either to zero, or to the dummy address?) to then set >>> values when appropriate, and reduce the number of calls to >>> rkisp1_write() to a single set of three after the processing. >>> >>> It might make the function simpler, and more readable, but it's more >>> effort, and this does look like it will solve the greyscale format issue >>> as discussed in earlier threads so I'd leave it up to you if you want to >>> refactor. >> >> Hi, >> Yes, I'll do that. >> Interestingly I found out that the patch causing the iommu page fault is >> >> https://www.spinics.net/lists/linux-media/msg176089.html >> >> Before that patch there are no iommu page faults but the video is corrupted. >> >> I can't explain how I didn't find it before, I clearly remember testing the grey format. > > It seems really weird indeed. > > Are you getting IOMMU faults on both the main and self paths ? Yes, both pathes. I get the page faults when checking out to this commit. Maybe when I tested back then, the iommu was somehow disabled? > >>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> >>>> /* >>>> * Use the dummy space allocated by dma_alloc_coherent to >
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index 768987d5f2dd..8e982dd0c740 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c @@ -249,7 +249,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_GREY, .uv_swap = 0, .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA, - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400, + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, .mbus = MEDIA_BUS_FMT_YUYV8_2X8, }, /* rgb */ @@ -631,12 +631,26 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) rkisp1_write(cap->rkisp1, buff_addr[RKISP1_PLANE_Y], cap->config->mi.y_base_ad_init); - rkisp1_write(cap->rkisp1, - buff_addr[RKISP1_PLANE_CB], - cap->config->mi.cb_base_ad_init); - rkisp1_write(cap->rkisp1, - buff_addr[RKISP1_PLANE_CR], - cap->config->mi.cr_base_ad_init); + /* + * In order to support grey format we capture + * YUV422 planar format from the camera and + * set the U and V planes to the dummy buffer + */ + if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { + rkisp1_write(cap->rkisp1, + cap->buf.dummy.dma_addr, + cap->config->mi.cb_base_ad_init); + rkisp1_write(cap->rkisp1, + cap->buf.dummy.dma_addr, + cap->config->mi.cr_base_ad_init); + } else { + rkisp1_write(cap->rkisp1, + buff_addr[RKISP1_PLANE_CB], + cap->config->mi.cb_base_ad_init); + rkisp1_write(cap->rkisp1, + buff_addr[RKISP1_PLANE_CR], + cap->config->mi.cr_base_ad_init); + } } else { /* * Use the dummy space allocated by dma_alloc_coherent to
Currently capturing grey format produces page faults on both selfpath and mainpath. To support greyscale we can capture YUV422 planar format and configure the U, V buffers to the dummy buffer. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- This is v2 of the patch "media: rkisp1: remove support for V4L2_PIX_FMT_GREY" In v1 I removed the grey format. In this version it is 'fixed' .../platform/rockchip/rkisp1/rkisp1-capture.c | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-)