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

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

On 23.01.22 18:55, Dafna Hirschfeld wrote:
> 
> 
> On 23.01.22 18:31, Dafna Hirschfeld wrote:
>>
>>
>> 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?
> 
> I see now that it works "fine" if CONFIG_ROCKCHIP_IOMMU is turned off, so
> it might be that I tested without iommu, though it is set in arch/arm64/configs/defconfig

I have a script that iterates all supported formats and stream them.
I see that when running the script, streaming gray format succeed, but when
testing grey format alone I get those page faults. I guess somehow buffers happen
to be mapped when running the script so it misses the faults.

> 
>>
>>>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>>
>>>>>>                   /*
>>>>>>                    * Use the dummy space allocated by dma_alloc_coherent to
>>>
>>