Re: [PATCH] media: rcar-vin: Add support for RAW10

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Laurent,

Thanks for your review.

On 2024-06-18 17:51:13 +0300, Laurent Pinchart wrote:
> On Tue, Jun 18, 2024 at 05:41:03PM +0300, Laurent Pinchart wrote:
> > Hi Geert,
> > 
> > On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote:
> > > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote:
> > > > Some R-Car SoCs are capable of capturing RAW10. Add support for it
> > > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct
> > > > format to express RAW10 unpacked to users.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > Thanks for your patch!
> > > 
> > > I am no VIN or V4L2 expert, but the register bits LGTM, so
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > 
> > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > >         case MEDIA_BUS_FMT_Y8_1X8:
> > > >                 vnmc |= VNMC_INF_RAW8;
> > > >                 break;
> > > > +       case MEDIA_BUS_FMT_Y10_1X10:
> > > > +               vnmc |= VNMC_INF_RGB666;
> > > 
> > > The actual meaning of this bit is not uniform across all SoCs.
> > > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp.
> > 
> > The INF bits have different meanings depending on the VIN input. What
> > you refer to above for V3U is for the CSI-2 input, while for the rest of
> > Gen3 you quote the values for the parallel input. Value 111 is
> > documented as "prohibit" for the CSI-2 input on the rest of Gen3.
> 
> To be precise, for V3U the documentation indicates "Input from Channel
> Selector", not CSI-2. V3U has no parallel input.

Yes it's getting a tad complex, but there is no issue here is there?  
This patch extends struct struct rvin_info with a new raw10 bool which 
indicates if raw10 is supported, or not. If it's not supported the 
driver rejects the MEDIA_BUS_FMT_Y10_1X10 in format validation.

> 
> The macros for the INF bits mix names for different types of inputs, it
> could be a good idea to clean this up.

There are so many things in this driver I would like to clean up and are 
working on. The first step is to clean up the async and VIN group mess, 
there are patches for that on the list. Once that is done I'm planing to 
refactor the init functions and defines, one per generation in different 
files to make it more clear how things look on the different generations.

> 
> > > > +               break;
> > > >         default:
> > > >                 break;
> > > >         }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund