Re: [PATCH v2] media: rkisp1: fix grey format iommu page faults

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart