Re: [PATCH v2 3/3] media: i2c: gc2145: Galaxy Core GC2145 sensor support

[Alain Volmat <alain.volmat@foss.st.com>]

Hi,

On Tue, Oct 31, 2023 at 09:05:32AM +0100, Jacopo Mondi wrote:
> Hi Alain
> 
> On Mon, Oct 30, 2023 at 05:37:11PM +0100, Alain Volmat wrote:
> > Hi Jacopo,
> >
> > On Mon, Oct 23, 2023 at 10:38:59AM +0200, Jacopo Mondi wrote:
> > > Hi Alain
> > >
> > > On Wed, Oct 11, 2023 at 07:57:30PM +0200, Alain Volmat wrote:
> > > > Addition of support for the Galaxy Core GC2145 XVGA sensor.
> > > > The sensor supports both DVP and CSI-2 interfaces however for
> > > > the time being only CSI-2 is implemented.
> > > >
> > > > Configurations is currently based on initialization scripts
> > > > coming from Galaxy Core and for that purpose only 3 static
> > > > resolutions are supported with static framerates.
> > > >  - 640x480 (30fps)
> > > >  - 1280x720 (30fps)
> > > >  - 1600x1200 (20fps)
> > >
> > > Anything blocking having a writable VBLANK ? This is a YUV sensor but
> > > GC2145_REG_VBLANK seems to be writable. I don't want to push you to
> > > more work that what you actually need, but configurable blankings (and
> > > then frame durations) seems like an important feature ? (and if I
> > > recall right you want to use this sensor with libcamera, which will
> > > require blankings to be controllable (if the sensor supports any RAW
> > > format)
> >
> > No, nothing prevents to write the VBLANK register.  I just did some
> > tests directly via rwmem into a running sensor and vertical blanking can
> > be updated, allowing to tune the framerate.
> >
> > >
> > > I don't see any RAW format being supported in this version. Is this
> > > something you plan to do on top ?
> >
> > Yes, absolutely, it is possible to output RAW formats as well however
> > this version of the driver doesn't support it yet.  The plan is indeed
> > to add it on top of this.
> > Several things to be addressed on top of this serie:
> >   - RAW format
> >   - frame_interval vs H/V blank control.  Is my understanding correct if
> >     I say that if a sensor has RAW format (even if it ALSO has YUV /
> >     RGB) then control is done via H/V blanking controls rather than
> >     frame_interval ?
> 
> I'll reply here to this question that is asked in a few other places.
> 
> I can only point you to the ov5640 driver, which is capable of both
> YUV/RGB and RAW as this sensor is. The ov5640 driver supports both the
> g/s/enum_frame_interval and has writable blankings. I guess it's more
> for historical reasons, as when blankings have been made writable
> users of the frame_interval API would have been displeased if that
> interface went away.
> 
> The resulting implementation is not nice, as changing vblank doesn't
> update the framerate reported through g_frame_interval, and keeping
> the two in sync is not trivial.
> 
> I would suggest to go for writable blankings, and if you already plan
> to remove frame_interval then not add it in first place so there won't
> be displeased users.
> 
> Sakari, Laurent, what's your opinion here ?

Sakari, Laurent, do you have opinion regarding how to control the
framerate on this sensor.  It is a YUV sensor but which might also (not
yet made available in this first serie) support RAW format.
Should I expose the g_frame_interval / s_frame_interval or only blanking
ctrls ?

> 
> 
> >   - parallel interface support
> >
> > >
> > > >
> 
> [snip]
> 
> > > > +/**
> > > > + * struct gc2145_format - GC2145 pixel format description
> > > > + * @code: media bus (MBUS) associated code
> > > > + * @colorspace: V4L2 colospace
> > > > + * @datatype: MIPI CSI2 data type
> > > > + * @output_fmt: GC2145 output format
> > > > + */
> > > > +struct gc2145_format {
> > > > +	unsigned int code;
> > > > +	unsigned int colorspace;
> > > > +	unsigned char datatype;
> > > > +	unsigned char output_fmt;
> > > > +};
> > > > +
> > > > +/* All supported formats */
> > > > +static const struct gc2145_format supported_formats[] = {
> > > > +	{
> > > > +		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
> > >
> > > The driver supports CSI-2, the 1X16 format variants should be used for
> > > serial bus
> >
> > Yes.  Doing this, this actually triggered big questioning since it seems
> > that the sensor, even in CSI, seems to be sending the RGB565 in
> > big-endian format ;-( I have just sent an email to GalaxyCore to clarify
> > this point however, once captured, I need to swap bytes in order to get
> > the right colors ;-( This is the reason why I used the RGB565_2X8_BE in
> > the first place and I believe this is correct for parallel mode, but my
> > understanding of the CSI formats makes me think that the sensor should
> > be sending the data differently.
> > I will wait for GalaxyCore reply before sending the v3.
> >
> 
> Let's wait for GC to get back with more information then, it still
> feels weird that a CSI-2 compliant sensor sends data out in a way
> different from what is described in the spec..

Format fixed in v3, got reply from GC, there were a bit in order to allow
switching the first/second cycle (if referring to parallel way to
transmit).  With that done, MIPI transmitted data are in LE.

Regards,
Alain