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

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

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
>