mbox series

[0/5] media: rkisp1: Fix LSC initial configuration on i.MX8MP

Message ID 20220817021850.20460-1-laurent.pinchart@ideasonboard.com (mailing list archive)
Headers show
Series media: rkisp1: Fix LSC initial configuration on i.MX8MP | expand

Message

Laurent Pinchart Aug. 17, 2022, 2:18 a.m. UTC
Hello,

This patch series fixes the Lens Shading Correction initial
configuration on the i.MX8MP.

The i.MX8MP integrates an ISP8000Nano v18.02, which unlike other
versions currently supported by the driver, gates access to the LSC RAM
with the ISP_CTRL.ISP_ENABLE bit. The initial LSC configuration being
performed before the ISP gets enabled, the writes to the RAM are
ignored, leading to incorrect results.

The series starts with four small drive-by cleanups of the LSC code, and
patch 5/5 then fixes the issue. I'm not totally thrilled by the code
architecture, but I'm not sure why, and I have a feeling doing better
would require a large refactoring of the ISP parameters handling. If
anyone sees an option for a better implementation, please say so.

The series is based on top of "[PATCH 0/7] media: rkisp1: Fix and
improve color space support" ([1]). Reviews for that base series would
thus be appreciated too.

[1] https://lore.kernel.org/linux-media/20220815065235.23797-1-laurent.pinchart@ideasonboard.com

Laurent Pinchart (5):
  media: rkisp1: Clean up LSC configuration code
  media: rkisp1: Store LSC register values in u32 variables
  media: rkisp1: Simplify LSC x/y size and grad register macros
  media: rkisp1: Use RKISP1_CIF_ISP_LSC_GRAD_SIZE() for gradient
    registers
  media: rkisp1: Configure LSC after enabling the ISP

 .../platform/rockchip/rkisp1/rkisp1-common.h  |  29 +-
 .../platform/rockchip/rkisp1/rkisp1-isp.c     |   9 +-
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 378 ++++++++++--------
 .../platform/rockchip/rkisp1/rkisp1-regs.h    |  20 +-
 4 files changed, 239 insertions(+), 197 deletions(-)

Comments

Dafna Hirschfeld Sept. 2, 2022, 9:07 a.m. UTC | #1
On 17.08.2022 05:18, Laurent Pinchart wrote:
>Hello,
>
>This patch series fixes the Lens Shading Correction initial
>configuration on the i.MX8MP.
>
>The i.MX8MP integrates an ISP8000Nano v18.02, which unlike other
>versions currently supported by the driver, gates access to the LSC RAM
>with the ISP_CTRL.ISP_ENABLE bit. The initial LSC configuration being
>performed before the ISP gets enabled, the writes to the RAM are
>ignored, leading to incorrect results.
>
>The series starts with four small drive-by cleanups of the LSC code, and
>patch 5/5 then fixes the issue. I'm not totally thrilled by the code
>architecture, but I'm not sure why, and I have a feeling doing better
>would require a large refactoring of the ISP parameters handling. If
>anyone sees an option for a better implementation, please say so.
>
>The series is based on top of "[PATCH 0/7] media: rkisp1: Fix and
>improve color space support" ([1]). Reviews for that base series would
>thus be appreciated too.
>
>[1] https://lore.kernel.org/linux-media/20220815065235.23797-1-laurent.pinchart@ideasonboard.com

Hi, I see that the series is also based on "[PATCH v2 00/55] media: rkisp1: Cleanups and add support"
right? I could not apply patch 5/5 because it seems to sit on top of
'[PATCH v2 49/55] media: rkisp1: Configure gasket on i.MX8MP'
Do you have a branch you can share with all the sets?

Thanks,
Dafna

>
>Laurent Pinchart (5):
>  media: rkisp1: Clean up LSC configuration code
>  media: rkisp1: Store LSC register values in u32 variables
>  media: rkisp1: Simplify LSC x/y size and grad register macros
>  media: rkisp1: Use RKISP1_CIF_ISP_LSC_GRAD_SIZE() for gradient
>    registers
>  media: rkisp1: Configure LSC after enabling the ISP
>
> .../platform/rockchip/rkisp1/rkisp1-common.h  |  29 +-
> .../platform/rockchip/rkisp1/rkisp1-isp.c     |   9 +-
> .../platform/rockchip/rkisp1/rkisp1-params.c  | 378 ++++++++++--------
> .../platform/rockchip/rkisp1/rkisp1-regs.h    |  20 +-
> 4 files changed, 239 insertions(+), 197 deletions(-)
>
>-- 
>Regards,
>
>Laurent Pinchart
>
Laurent Pinchart Sept. 2, 2022, 10:07 a.m. UTC | #2
Hi Dafna,

On Fri, Sep 02, 2022 at 12:07:04PM +0300, Dafna Hirschfeld wrote:
> On 17.08.2022 05:18, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series fixes the Lens Shading Correction initial
> > configuration on the i.MX8MP.
> > 
> > The i.MX8MP integrates an ISP8000Nano v18.02, which unlike other
> > versions currently supported by the driver, gates access to the LSC RAM
> > with the ISP_CTRL.ISP_ENABLE bit. The initial LSC configuration being
> > performed before the ISP gets enabled, the writes to the RAM are
> > ignored, leading to incorrect results.
> > 
> > The series starts with four small drive-by cleanups of the LSC code, and
> > patch 5/5 then fixes the issue. I'm not totally thrilled by the code
> > architecture, but I'm not sure why, and I have a feeling doing better
> > would require a large refactoring of the ISP parameters handling. If
> > anyone sees an option for a better implementation, please say so.
> > 
> > The series is based on top of "[PATCH 0/7] media: rkisp1: Fix and
> > improve color space support" ([1]). Reviews for that base series would
> > thus be appreciated too.
> > 
> > [1] https://lore.kernel.org/linux-media/20220815065235.23797-1-laurent.pinchart@ideasonboard.com
> 
> Hi, I see that the series is also based on "[PATCH v2 00/55] media: rkisp1: Cleanups and add support"
> right? I could not apply patch 5/5 because it seems to sit on top of
> '[PATCH v2 49/55] media: rkisp1: Configure gasket on i.MX8MP'
> Do you have a branch you can share with all the sets?

Sure. You can find all pending patches for the rkisp1 at
https://gitlab.com/ideasonboard/nxp/linux/-/tree/pinchartl/v6.0/isp.
Commits up to and including "media: rkisp1: Zero v4l2_subdev_format
fields in when validating links" are candidates for v6.1, commits
starting at "dt-bindings: media: rkisp1: Add i.MX8MP ISP to compatible"
need some more work.

> > Laurent Pinchart (5):
> >   media: rkisp1: Clean up LSC configuration code
> >   media: rkisp1: Store LSC register values in u32 variables
> >   media: rkisp1: Simplify LSC x/y size and grad register macros
> >   media: rkisp1: Use RKISP1_CIF_ISP_LSC_GRAD_SIZE() for gradient
> >     registers
> >   media: rkisp1: Configure LSC after enabling the ISP
> > 
> >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  29 +-
> >  .../platform/rockchip/rkisp1/rkisp1-isp.c     |   9 +-
> >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 378 ++++++++++--------
> >  .../platform/rockchip/rkisp1/rkisp1-regs.h    |  20 +-
> >  4 files changed, 239 insertions(+), 197 deletions(-)