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

[Sakari Ailus]

Hi Alain, Jacopo,

On Mon, Nov 06, 2023 at 10:55:46AM +0100, Alain Volmat wrote:
> 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 ?

As this is a new driver, I'm leaning towards implementing blanking controls
only.

-- 
Regards,

Sakari Ailus