[PATCH 0/2] staging: media: imx7-mipi-csis: Two small fixes

[PATCH 0/2] staging: media: imx7-mipi-csis: Two small fixes

[Jacopo Mondi]

Two small fixes for the imx7-mipi-csis driver.

The second patch is required to correctly capture in packed YUV422 formats

Thanks
   j

Jacopo Mondi (2):
  staging: media: imx: imx7-mipi-csic: Resume on debug
  staging: media: imx: imx7-mipi-csic: Set PIXEL_MODE for YUV422

 drivers/staging/media/imx/imx7-mipi-csis.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--
2.34.1

[PATCH 1/2] staging: media: imx: imx7-mipi-csic: Resume on debug

[Jacopo Mondi]

The mipi_csis_dump_regs() function reads and printout the interface
registers for debugging purposes.

Trying to access the registers without proper powering up the interface
causes the chip to hang.

Fix that by increasing the pm runtime usage count which, if necessary,
resumes the interface.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 2b73fa55c938..cb54bb7491d9 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state)
 
 	dev_info(state->dev, "--- REGISTERS ---\n");
 
+	pm_runtime_resume_and_get(state->dev);
+
 	for (i = 0; i < ARRAY_SIZE(registers); i++) {
 		cfg = mipi_csis_read(state, registers[i].offset);
 		dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
 	}
 
+	pm_runtime_put(state->dev);
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/2] staging: media: imx: imx7-mipi-csic: Set PIXEL_MODE for YUV422

[Jacopo Mondi]

Bits 13 and 12 of the ISP_CONFIGn register configure the PIXEL_MODE
which specifies the sampling size, in pixel component units, on the
CSI-2 output data interface when data are transferred to memory.

The register description in the chip manual specifies that DUAL mode
should be used for YUV422 data but does not clarify the reason.

Todo: verify if other YUV formats require the same setting and what is the
appropriate setting for RAW and sRGB formats.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index cb54bb7491d9..e96989c46479 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -499,6 +499,10 @@ static void __mipi_csis_set_format(struct csi_state *state)
 	/* Color format */
 	val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
 	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
+
+	if (state->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
+		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
+
 	val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
 	mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
 
-- 
2.34.1

Re: [PATCH 1/2] staging: media: imx: imx7-mipi-csic: Resume on debug

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote:
> The mipi_csis_dump_regs() function reads and printout the interface
> registers for debugging purposes.
> 
> Trying to access the registers without proper powering up the interface
> causes the chip to hang.
> 
> Fix that by increasing the pm runtime usage count which, if necessary,
> resumes the interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 2b73fa55c938..cb54bb7491d9 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state)
>  
>  	dev_info(state->dev, "--- REGISTERS ---\n");
>  
> +	pm_runtime_resume_and_get(state->dev);

Should this have an error check ? With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
>  		cfg = mipi_csis_read(state, registers[i].offset);
>  		dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
>  	}
>  
> +	pm_runtime_put(state->dev);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] staging: media: imx: imx7-mipi-csic: Set PIXEL_MODE for YUV422

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 19, 2022 at 12:20:24PM +0100, Jacopo Mondi wrote:
> Bits 13 and 12 of the ISP_CONFIGn register configure the PIXEL_MODE
> which specifies the sampling size, in pixel component units, on the
> CSI-2 output data interface when data are transferred to memory.
> 
> The register description in the chip manual specifies that DUAL mode
> should be used for YUV422 data but does not clarify the reason.
> 
> Todo: verify if other YUV formats require the same setting and what is the
> appropriate setting for RAW and sRGB formats.

I'll also test it on i.MX8MM, I recall I had to *not* set
PIXEL_MODE_DUAL on that platform for YUV to work.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index cb54bb7491d9..e96989c46479 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -499,6 +499,10 @@ static void __mipi_csis_set_format(struct csi_state *state)
>  	/* Color format */
>  	val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>  	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> +
> +	if (state->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
> +		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
> +
>  	val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
>  	mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/2] staging: media: imx7-mipi-csis: Two small fixes

[Rui Miguel Silva]

Hey Jacopo,
both patches LGTM.

I only have one nit with them...

On Wed Jan 19, 2022 at 11:20 AM WET, Jacopo Mondi wrote:

> Two small fixes for the imx7-mipi-csis driver.
>
> The second patch is required to correctly capture in packed YUV422 formats
>
> Thanks
>    j
>
> Jacopo Mondi (2):
>   staging: media: imx: imx7-mipi-csic: Resume on debug
>   staging: media: imx: imx7-mipi-csic: Set PIXEL_MODE for YUV422
                                      ^
                   here should be s (imx7-mipi-csis) on both patches

Maybe Laurent could fix the typo on the pull, other than that:

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Cheers,
   Rui

>
>  drivers/staging/media/imx/imx7-mipi-csis.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> --
> 2.34.1

Re: [PATCH 0/2] staging: media: imx7-mipi-csis: Two small fixes

[Jacopo Mondi]

Hi Rui

On Wed, Jan 19, 2022 at 04:21:15PM +0000, Rui Miguel Silva wrote:
> Hey Jacopo,
> both patches LGTM.
>
> I only have one nit with them...
>
> On Wed Jan 19, 2022 at 11:20 AM WET, Jacopo Mondi wrote:
>
> > Two small fixes for the imx7-mipi-csis driver.
> >
> > The second patch is required to correctly capture in packed YUV422 formats
> >
> > Thanks
> >    j
> >
> > Jacopo Mondi (2):
> >   staging: media: imx: imx7-mipi-csic: Resume on debug
> >   staging: media: imx: imx7-mipi-csic: Set PIXEL_MODE for YUV422
>                                       ^
>                    here should be s (imx7-mipi-csis) on both patches

Oh ups, I honestly thought at the time the IP core was called csic...

>
> Maybe Laurent could fix the typo on the pull, other than that:

I owe an update to add an error check to 1/2 once Laurent (or someone
with an i.MX8MM) tests 2/2, so I can fix the subject in v2.

>
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Thanks
   j

>
> Cheers,
>    Rui
>
> >
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > --
> > 2.34.1
>
>
>

Re: [PATCH 1/2] staging: media: imx: imx7-mipi-csic: Resume on debug

[Laurent Pinchart]

Hi Jacopo,

On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote:
> On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > The mipi_csis_dump_regs() function reads and printout the interface
> > registers for debugging purposes.
> > 
> > Trying to access the registers without proper powering up the interface
> > causes the chip to hang.
> > 
> > Fix that by increasing the pm runtime usage count which, if necessary,
> > resumes the interface.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 2b73fa55c938..cb54bb7491d9 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state)
> >  
> >  	dev_info(state->dev, "--- REGISTERS ---\n");
> >  
> > +	pm_runtime_resume_and_get(state->dev);
> 
> Should this have an error check ? With that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I just noticed that the call to mipi_csis_dump_regs() in
mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An
alternative would thus be to add the same condition to
mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as
dumping the register when then hardware is turned off is quite
pointeless. Up to you.

> > +
> >  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> >  		cfg = mipi_csis_read(state, registers[i].offset);
> >  		dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
> >  	}
> >  
> > +	pm_runtime_put(state->dev);
> > +
> >  	return 0;
> >  }
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] staging: media: imx: imx7-mipi-csic: Resume on debug

[Jacopo Mondi]

Hi Laurent,

On Tue, Jan 25, 2022 at 05:18:26AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > The mipi_csis_dump_regs() function reads and printout the interface
> > > registers for debugging purposes.
> > >
> > > Trying to access the registers without proper powering up the interface
> > > causes the chip to hang.
> > >
> > > Fix that by increasing the pm runtime usage count which, if necessary,
> > > resumes the interface.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > > index 2b73fa55c938..cb54bb7491d9 100644
> > > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state)
> > >
> > >  	dev_info(state->dev, "--- REGISTERS ---\n");
> > >
> > > +	pm_runtime_resume_and_get(state->dev);
> >
> > Should this have an error check ? With that,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I just noticed that the call to mipi_csis_dump_regs() in
> mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An
> alternative would thus be to add the same condition to
> mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as
> dumping the register when then hardware is turned off is quite
> pointeless. Up to you.
>

Tbh, I would drop this custom sysfs attribute completely.
It should serve the purpose to easily dump the reg value, but it is
either accessed at the right time (ie during the streaming session)
otherwise all you get is POR default values (or a hang, without this
patch)

> > > +
> > >  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> > >  		cfg = mipi_csis_read(state, registers[i].offset);
> > >  		dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
> > >  	}
> > >
> > > +	pm_runtime_put(state->dev);
> > > +
> > >  	return 0;
> > >  }
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] staging: media: imx: imx7-mipi-csic: Resume on debug

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jan 25, 2022 at 11:22:57AM +0100, Jacopo Mondi wrote:
> On Tue, Jan 25, 2022 at 05:18:26AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote:
> > > On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > > The mipi_csis_dump_regs() function reads and printout the interface
> > > > registers for debugging purposes.
> > > >
> > > > Trying to access the registers without proper powering up the interface
> > > > causes the chip to hang.
> > > >
> > > > Fix that by increasing the pm runtime usage count which, if necessary,
> > > > resumes the interface.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > > > index 2b73fa55c938..cb54bb7491d9 100644
> > > > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > > > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > > > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state)
> > > >
> > > >  	dev_info(state->dev, "--- REGISTERS ---\n");
> > > >
> > > > +	pm_runtime_resume_and_get(state->dev);
> > >
> > > Should this have an error check ? With that,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > I just noticed that the call to mipi_csis_dump_regs() in
> > mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An
> > alternative would thus be to add the same condition to
> > mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as
> > dumping the register when then hardware is turned off is quite
> > pointeless. Up to you.
> 
> Tbh, I would drop this custom sysfs attribute completely.
> It should serve the purpose to easily dump the reg value, but it is
> either accessed at the right time (ie during the streaming session)
> otherwise all you get is POR default values (or a hang, without this
> patch)

The debugfs interface has served me before to diagnose problems, as it
allows checking how the register values change during streaming, in
particular the DPHY status. I'd like to keep it if possible.

> > > > +
> > > >  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> > > >  		cfg = mipi_csis_read(state, registers[i].offset);
> > > >  		dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
> > > >  	}
> > > >
> > > > +	pm_runtime_put(state->dev);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >

-- 
Regards,

Laurent Pinchart