Re: [PATCH 3/3] media: rcar-vin: Fix RAW10

[Tomi Valkeinen]

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