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

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

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
>>