| Message ID | 20250324-rcar-fix-raw-v1-3-ae56c1c7a2f6@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: rcar: Fix RAW formats | expand |
Hi Tomi, Thanks for your work. On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: > Fix the following to get RAW10 formats working: > > In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to > 16-bit containers, the bpp should be 2. > > Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data > Through Mode", used for YUV formats and should not be set for RAW10. > > Fix the check related to the RGB666 format and CSI-2 mode. The > VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 > is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). > This feels a bit hacky, though, and the formats should really have been > verified already earlier. I agree, it feels hacky. I would rather just remove the while switch then try to "fix" it by extending it more. When testing this series I needed a similar fix for VNMC_INF_RAW8 check below to get it to work on Gen4. > > Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- > drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > index 53046614f7a1..f8394be8a922 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) > case VNMC_INF_YUV8_BT656: > case VNMC_INF_YUV10_BT656: > case VNMC_INF_YUV16: > - case VNMC_INF_RGB666: > if (vin->is_csi) { > vin_err(vin, "Invalid setting in MIPI CSI2\n"); > return -EINVAL; > } > break; > + case VNMC_INF_RGB666: > + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { > + vin_err(vin, "Invalid setting in MIPI CSI2\n"); > + return -EINVAL; > + } > + break; > case VNMC_INF_RAW8: > if (!vin->is_csi) { > vin_err(vin, "Invalid setting in Digital Pins\n"); > @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) > case V4L2_PIX_FMT_SGBRG10: > case V4L2_PIX_FMT_SGRBG10: > case V4L2_PIX_FMT_SRGGB10: > - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; > + dmr = VNDMR_RMODE_RAW10; > break; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > index 756fdfdbce61..65da8d513b52 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { > }, > { > .fourcc = V4L2_PIX_FMT_SBGGR10, > - .bpp = 4, > + .bpp = 2, > }, > { > .fourcc = V4L2_PIX_FMT_SGBRG10, > - .bpp = 4, > + .bpp = 2, > }, > { > .fourcc = V4L2_PIX_FMT_SGRBG10, > - .bpp = 4, > + .bpp = 2, > }, > { > .fourcc = V4L2_PIX_FMT_SRGGB10, > - .bpp = 4, > + .bpp = 2, > }, > }; > > > -- > 2.43.0 >
Hi, On 01/04/2025 00:11, Niklas Söderlund wrote: > Hi Tomi, > > Thanks for your work. > > On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: >> Fix the following to get RAW10 formats working: >> >> In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to >> 16-bit containers, the bpp should be 2. >> >> Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data >> Through Mode", used for YUV formats and should not be set for RAW10. >> >> Fix the check related to the RGB666 format and CSI-2 mode. The >> VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 >> is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). >> This feels a bit hacky, though, and the formats should really have been >> verified already earlier. > > I agree, it feels hacky. I would rather just remove the while switch > then try to "fix" it by extending it more. When testing this series I > needed a similar fix for VNMC_INF_RAW8 check below to get it to work on > Gen4. Why is that? What is VNMC_INF_RAW8 overlapping with? Tomi >> >> Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- >> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> index 53046614f7a1..f8394be8a922 100644 >> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) >> case VNMC_INF_YUV8_BT656: >> case VNMC_INF_YUV10_BT656: >> case VNMC_INF_YUV16: >> - case VNMC_INF_RGB666: >> if (vin->is_csi) { >> vin_err(vin, "Invalid setting in MIPI CSI2\n"); >> return -EINVAL; >> } >> break; >> + case VNMC_INF_RGB666: >> + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { >> + vin_err(vin, "Invalid setting in MIPI CSI2\n"); >> + return -EINVAL; >> + } >> + break; >> case VNMC_INF_RAW8: >> if (!vin->is_csi) { >> vin_err(vin, "Invalid setting in Digital Pins\n"); >> @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) >> case V4L2_PIX_FMT_SGBRG10: >> case V4L2_PIX_FMT_SGRBG10: >> case V4L2_PIX_FMT_SRGGB10: >> - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; >> + dmr = VNDMR_RMODE_RAW10; >> break; >> default: >> vin_err(vin, "Invalid pixelformat (0x%x)\n", >> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> index 756fdfdbce61..65da8d513b52 100644 >> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { >> }, >> { >> .fourcc = V4L2_PIX_FMT_SBGGR10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SGBRG10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SGRBG10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SRGGB10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> }; >> >> >> -- >> 2.43.0 >> >
Hi, On 01/04/2025 00:11, Niklas Söderlund wrote: > Hi Tomi, > > Thanks for your work. > > On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: >> Fix the following to get RAW10 formats working: >> >> In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to >> 16-bit containers, the bpp should be 2. >> >> Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data >> Through Mode", used for YUV formats and should not be set for RAW10. >> >> Fix the check related to the RGB666 format and CSI-2 mode. The >> VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 >> is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). >> This feels a bit hacky, though, and the formats should really have been >> verified already earlier. > > I agree, it feels hacky. I would rather just remove the while switch > then try to "fix" it by extending it more. When testing this series I > needed a similar fix for VNMC_INF_RAW8 check below to get it to work on > Gen4. Also, I think it's fine to be a bit hacky to have a small fix for a feature already in upstream, instead of doing a bigger restructuring to fix it. All this should be cleaned up, but in my opinion it's better to do that on top of a working upstream. Tomi >> >> Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- >> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> index 53046614f7a1..f8394be8a922 100644 >> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >> @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) >> case VNMC_INF_YUV8_BT656: >> case VNMC_INF_YUV10_BT656: >> case VNMC_INF_YUV16: >> - case VNMC_INF_RGB666: >> if (vin->is_csi) { >> vin_err(vin, "Invalid setting in MIPI CSI2\n"); >> return -EINVAL; >> } >> break; >> + case VNMC_INF_RGB666: >> + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { >> + vin_err(vin, "Invalid setting in MIPI CSI2\n"); >> + return -EINVAL; >> + } >> + break; >> case VNMC_INF_RAW8: >> if (!vin->is_csi) { >> vin_err(vin, "Invalid setting in Digital Pins\n"); >> @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) >> case V4L2_PIX_FMT_SGBRG10: >> case V4L2_PIX_FMT_SGRBG10: >> case V4L2_PIX_FMT_SRGGB10: >> - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; >> + dmr = VNDMR_RMODE_RAW10; >> break; >> default: >> vin_err(vin, "Invalid pixelformat (0x%x)\n", >> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> index 756fdfdbce61..65da8d513b52 100644 >> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >> @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { >> }, >> { >> .fourcc = V4L2_PIX_FMT_SBGGR10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SGBRG10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SGRBG10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> { >> .fourcc = V4L2_PIX_FMT_SRGGB10, >> - .bpp = 4, >> + .bpp = 2, >> }, >> }; >> >> >> -- >> 2.43.0 >> >
Hi Tomi, On 2025-04-03 11:15:04 +0300, Tomi Valkeinen wrote: > Hi, > > On 01/04/2025 00:11, Niklas Söderlund wrote: > > Hi Tomi, > > > > Thanks for your work. > > > > On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: > > > Fix the following to get RAW10 formats working: > > > > > > In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to > > > 16-bit containers, the bpp should be 2. > > > > > > Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data > > > Through Mode", used for YUV formats and should not be set for RAW10. > > > > > > Fix the check related to the RGB666 format and CSI-2 mode. The > > > VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 > > > is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). > > > This feels a bit hacky, though, and the formats should really have been > > > verified already earlier. > > > > I agree, it feels hacky. I would rather just remove the while switch > > then try to "fix" it by extending it more. When testing this series I > > needed a similar fix for VNMC_INF_RAW8 check below to get it to work on > > Gen4. > > Why is that? What is VNMC_INF_RAW8 overlapping with? VNMC_INF_RAW8 is not overlapping the issue is the "if (!vin->is_csi)" check. On Gen3 RAW8 was supposedly only supported on the CSI-2 input, and not on the parallel bus available on some VIN instances. On Gen4 the checks around vin->is_csi becomes skewed and as you point out should be cloned up. The effect at the moment however is that o Gen4 when trying to capture RAW8 is_csi is not set (as this is a gen3 thing) and the "Invalid setting in Digital Pins" error path is hit. Locally I have now removed this whole swtich, as you point out the formats should already be checked, and as far as I can tell they are. This check is a relic from early Gen3 support. And really proves after the Gen2 media graph clenups are done we should split rcar-dma.c -> rcar-dma-gen{2,3,4}.c to get rid of this mess ;-) I would change this patch to just remove the whole switch. If you want to keep this fix as is I'm ok with that. I can submit a patch removing the switch if you like on top. > > Tomi > > > > > > > Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > > --- > > > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- > > > drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- > > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > index 53046614f7a1..f8394be8a922 100644 > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) > > > case VNMC_INF_YUV8_BT656: > > > case VNMC_INF_YUV10_BT656: > > > case VNMC_INF_YUV16: > > > - case VNMC_INF_RGB666: > > > if (vin->is_csi) { > > > vin_err(vin, "Invalid setting in MIPI CSI2\n"); > > > return -EINVAL; > > > } > > > break; > > > + case VNMC_INF_RGB666: > > > + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { > > > + vin_err(vin, "Invalid setting in MIPI CSI2\n"); > > > + return -EINVAL; > > > + } > > > + break; > > > case VNMC_INF_RAW8: > > > if (!vin->is_csi) { > > > vin_err(vin, "Invalid setting in Digital Pins\n"); > > > @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) > > > case V4L2_PIX_FMT_SGBRG10: > > > case V4L2_PIX_FMT_SGRBG10: > > > case V4L2_PIX_FMT_SRGGB10: > > > - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; > > > + dmr = VNDMR_RMODE_RAW10; > > > break; > > > default: > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > index 756fdfdbce61..65da8d513b52 100644 > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { > > > }, > > > { > > > .fourcc = V4L2_PIX_FMT_SBGGR10, > > > - .bpp = 4, > > > + .bpp = 2, > > > }, > > > { > > > .fourcc = V4L2_PIX_FMT_SGBRG10, > > > - .bpp = 4, > > > + .bpp = 2, > > > }, > > > { > > > .fourcc = V4L2_PIX_FMT_SGRBG10, > > > - .bpp = 4, > > > + .bpp = 2, > > > }, > > > { > > > .fourcc = V4L2_PIX_FMT_SRGGB10, > > > - .bpp = 4, > > > + .bpp = 2, > > > }, > > > }; > > > > > > -- > > > 2.43.0 > > > > > >
Hi, On 03/04/2025 11:57, Niklas Söderlund wrote: > Hi Tomi, > > On 2025-04-03 11:15:04 +0300, Tomi Valkeinen wrote: >> Hi, >> >> On 01/04/2025 00:11, Niklas Söderlund wrote: >>> Hi Tomi, >>> >>> Thanks for your work. >>> >>> On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: >>>> Fix the following to get RAW10 formats working: >>>> >>>> In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to >>>> 16-bit containers, the bpp should be 2. >>>> >>>> Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data >>>> Through Mode", used for YUV formats and should not be set for RAW10. >>>> >>>> Fix the check related to the RGB666 format and CSI-2 mode. The >>>> VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 >>>> is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). >>>> This feels a bit hacky, though, and the formats should really have been >>>> verified already earlier. >>> >>> I agree, it feels hacky. I would rather just remove the while switch >>> then try to "fix" it by extending it more. When testing this series I >>> needed a similar fix for VNMC_INF_RAW8 check below to get it to work on >>> Gen4. >> >> Why is that? What is VNMC_INF_RAW8 overlapping with? > > VNMC_INF_RAW8 is not overlapping the issue is the "if (!vin->is_csi)" > check. On Gen3 RAW8 was supposedly only supported on the CSI-2 input, > and not on the parallel bus available on some VIN instances. > > On Gen4 the checks around vin->is_csi becomes skewed and as you point > out should be cloned up. The effect at the moment however is that o Gen4 > when trying to capture RAW8 is_csi is not set (as this is a gen3 thing) > and the "Invalid setting in Digital Pins" error path is hit. Ok. I was wondering why it worked for me. But maybe the reason it worked for me was the extra series from you, which also changed these checks to rvin_remote_is_parallel(). And perhaps that function works fine, while the older vin->is_csi didn't. > Locally I have now removed this whole swtich, as you point out the > formats should already be checked, and as far as I can tell they are. > This check is a relic from early Gen3 support. And really proves after > the Gen2 media graph clenups are done we should split rcar-dma.c -> > rcar-dma-gen{2,3,4}.c to get rid of this mess ;-) > > I would change this patch to just remove the whole switch. If you want > to keep this fix as is I'm ok with that. I can submit a patch removing > the switch if you like on top. Ok, if you think removing the switch is fine, we can do that. I wasn't sure of the flow in the driver so I wasn't confident I could just drop it (especially considering pre-gen4, with which I have no experience). Thus this patch. Tomi > >> >> Tomi >> >>>> >>>> Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >>>> --- >>>> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- >>>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- >>>> 2 files changed, 11 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >>>> index 53046614f7a1..f8394be8a922 100644 >>>> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >>>> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c >>>> @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) >>>> case VNMC_INF_YUV8_BT656: >>>> case VNMC_INF_YUV10_BT656: >>>> case VNMC_INF_YUV16: >>>> - case VNMC_INF_RGB666: >>>> if (vin->is_csi) { >>>> vin_err(vin, "Invalid setting in MIPI CSI2\n"); >>>> return -EINVAL; >>>> } >>>> break; >>>> + case VNMC_INF_RGB666: >>>> + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { >>>> + vin_err(vin, "Invalid setting in MIPI CSI2\n"); >>>> + return -EINVAL; >>>> + } >>>> + break; >>>> case VNMC_INF_RAW8: >>>> if (!vin->is_csi) { >>>> vin_err(vin, "Invalid setting in Digital Pins\n"); >>>> @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) >>>> case V4L2_PIX_FMT_SGBRG10: >>>> case V4L2_PIX_FMT_SGRBG10: >>>> case V4L2_PIX_FMT_SRGGB10: >>>> - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; >>>> + dmr = VNDMR_RMODE_RAW10; >>>> break; >>>> default: >>>> vin_err(vin, "Invalid pixelformat (0x%x)\n", >>>> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >>>> index 756fdfdbce61..65da8d513b52 100644 >>>> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >>>> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >>>> @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { >>>> }, >>>> { >>>> .fourcc = V4L2_PIX_FMT_SBGGR10, >>>> - .bpp = 4, >>>> + .bpp = 2, >>>> }, >>>> { >>>> .fourcc = V4L2_PIX_FMT_SGBRG10, >>>> - .bpp = 4, >>>> + .bpp = 2, >>>> }, >>>> { >>>> .fourcc = V4L2_PIX_FMT_SGRBG10, >>>> - .bpp = 4, >>>> + .bpp = 2, >>>> }, >>>> { >>>> .fourcc = V4L2_PIX_FMT_SRGGB10, >>>> - .bpp = 4, >>>> + .bpp = 2, >>>> }, >>>> }; >>>> >>>> -- >>>> 2.43.0 >>>> >>> >> >
Hi Tomi, On 2025-04-03 12:12:06 +0300, Tomi Valkeinen wrote: > Hi, > > On 03/04/2025 11:57, Niklas Söderlund wrote: > > Hi Tomi, > > > > On 2025-04-03 11:15:04 +0300, Tomi Valkeinen wrote: > > > Hi, > > > > > > On 01/04/2025 00:11, Niklas Söderlund wrote: > > > > Hi Tomi, > > > > > > > > Thanks for your work. > > > > > > > > On 2025-03-24 13:48:54 +0200, Tomi Valkeinen wrote: > > > > > Fix the following to get RAW10 formats working: > > > > > > > > > > In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to > > > > > 16-bit containers, the bpp should be 2. > > > > > > > > > > Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data > > > > > Through Mode", used for YUV formats and should not be set for RAW10. > > > > > > > > > > Fix the check related to the RGB666 format and CSI-2 mode. The > > > > > VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 > > > > > is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). > > > > > This feels a bit hacky, though, and the formats should really have been > > > > > verified already earlier. > > > > > > > > I agree, it feels hacky. I would rather just remove the while switch > > > > then try to "fix" it by extending it more. When testing this series I > > > > needed a similar fix for VNMC_INF_RAW8 check below to get it to work on > > > > Gen4. > > > > > > Why is that? What is VNMC_INF_RAW8 overlapping with? > > > > VNMC_INF_RAW8 is not overlapping the issue is the "if (!vin->is_csi)" > > check. On Gen3 RAW8 was supposedly only supported on the CSI-2 input, > > and not on the parallel bus available on some VIN instances. > > > > On Gen4 the checks around vin->is_csi becomes skewed and as you point > > out should be cloned up. The effect at the moment however is that o Gen4 > > when trying to capture RAW8 is_csi is not set (as this is a gen3 thing) > > and the "Invalid setting in Digital Pins" error path is hit. > > Ok. I was wondering why it worked for me. But maybe the reason it worked for > me was the extra series from you, which also changed these checks to > rvin_remote_is_parallel(). And perhaps that function works fine, while the > older vin->is_csi didn't. Ahh yes that might be it. There are so many VIN series floating around :-) > > > Locally I have now removed this whole swtich, as you point out the > > formats should already be checked, and as far as I can tell they are. > > This check is a relic from early Gen3 support. And really proves after > > the Gen2 media graph clenups are done we should split rcar-dma.c -> > > rcar-dma-gen{2,3,4}.c to get rid of this mess ;-) > > > > I would change this patch to just remove the whole switch. If you want > > to keep this fix as is I'm ok with that. I can submit a patch removing > > the switch if you like on top. > > Ok, if you think removing the switch is fine, we can do that. I wasn't sure > of the flow in the driver so I wasn't confident I could just drop it > (especially considering pre-gen4, with which I have no experience). Thus > this patch. > > Tomi > > > > > > > > > Tomi > > > > > > > > > > > > > Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > > > > --- > > > > > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- > > > > > drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- > > > > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > > index 53046614f7a1..f8394be8a922 100644 > > > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > > @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) > > > > > case VNMC_INF_YUV8_BT656: > > > > > case VNMC_INF_YUV10_BT656: > > > > > case VNMC_INF_YUV16: > > > > > - case VNMC_INF_RGB666: > > > > > if (vin->is_csi) { > > > > > vin_err(vin, "Invalid setting in MIPI CSI2\n"); > > > > > return -EINVAL; > > > > > } > > > > > break; > > > > > + case VNMC_INF_RGB666: > > > > > + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { > > > > > + vin_err(vin, "Invalid setting in MIPI CSI2\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > + break; > > > > > case VNMC_INF_RAW8: > > > > > if (!vin->is_csi) { > > > > > vin_err(vin, "Invalid setting in Digital Pins\n"); > > > > > @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) > > > > > case V4L2_PIX_FMT_SGBRG10: > > > > > case V4L2_PIX_FMT_SGRBG10: > > > > > case V4L2_PIX_FMT_SRGGB10: > > > > > - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; > > > > > + dmr = VNDMR_RMODE_RAW10; > > > > > break; > > > > > default: > > > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > > > index 756fdfdbce61..65da8d513b52 100644 > > > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > > > > > @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { > > > > > }, > > > > > { > > > > > .fourcc = V4L2_PIX_FMT_SBGGR10, > > > > > - .bpp = 4, > > > > > + .bpp = 2, > > > > > }, > > > > > { > > > > > .fourcc = V4L2_PIX_FMT_SGBRG10, > > > > > - .bpp = 4, > > > > > + .bpp = 2, > > > > > }, > > > > > { > > > > > .fourcc = V4L2_PIX_FMT_SGRBG10, > > > > > - .bpp = 4, > > > > > + .bpp = 2, > > > > > }, > > > > > { > > > > > .fourcc = V4L2_PIX_FMT_SRGGB10, > > > > > - .bpp = 4, > > > > > + .bpp = 2, > > > > > }, > > > > > }; > > > > > > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > > > > >
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c index 53046614f7a1..f8394be8a922 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c @@ -811,12 +811,17 @@ static int rvin_setup(struct rvin_dev *vin) case VNMC_INF_YUV8_BT656: case VNMC_INF_YUV10_BT656: case VNMC_INF_YUV16: - case VNMC_INF_RGB666: if (vin->is_csi) { vin_err(vin, "Invalid setting in MIPI CSI2\n"); return -EINVAL; } break; + case VNMC_INF_RGB666: + if (vin->info->model == RCAR_GEN3 && vin->is_csi) { + vin_err(vin, "Invalid setting in MIPI CSI2\n"); + return -EINVAL; + } + break; case VNMC_INF_RAW8: if (!vin->is_csi) { vin_err(vin, "Invalid setting in Digital Pins\n"); @@ -913,7 +918,7 @@ static int rvin_setup(struct rvin_dev *vin) case V4L2_PIX_FMT_SGBRG10: case V4L2_PIX_FMT_SGRBG10: case V4L2_PIX_FMT_SRGGB10: - dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; + dmr = VNDMR_RMODE_RAW10; break; default: vin_err(vin, "Invalid pixelformat (0x%x)\n", diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c index 756fdfdbce61..65da8d513b52 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c @@ -88,19 +88,19 @@ static const struct rvin_video_format rvin_formats[] = { }, { .fourcc = V4L2_PIX_FMT_SBGGR10, - .bpp = 4, + .bpp = 2, }, { .fourcc = V4L2_PIX_FMT_SGBRG10, - .bpp = 4, + .bpp = 2, }, { .fourcc = V4L2_PIX_FMT_SGRBG10, - .bpp = 4, + .bpp = 2, }, { .fourcc = V4L2_PIX_FMT_SRGGB10, - .bpp = 4, + .bpp = 2, }, };
Fix the following to get RAW10 formats working: In rvin_formats, the bpp is set to 4 for RAW10. As VIN unpacks RAW10 to 16-bit containers, the bpp should be 2. Don't set VNDMR_YC_THR to the VNDMR register. The YC_THR is "YC Data Through Mode", used for YUV formats and should not be set for RAW10. Fix the check related to the RGB666 format and CSI-2 mode. The VNMC_INF_RGB666 define is the same as used for RAW10 on Gen4, and RAW10 is allowed on CSI-2 (whereas RGB666 is not allowed on Gen3 on CSI-2). This feels a bit hacky, though, and the formats should really have been verified already earlier. Fixes: 1b7e7240eaf3 ("media: rcar-vin: Add support for RAW10") Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 9 +++++++-- drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++---- 2 files changed, 11 insertions(+), 6 deletions(-)