Hi Krzysztof
On Tue, Oct 12, 2021 at 02:24:17PM +0200, Krzysztof Hałasa wrote:
> Jacopo,
>
> > I'll pretend I haven't read this not to be dragged in tangential
> > discussions. My whole point was such a commit message is useless as I
> > assume if you submit a driver for inclusion you have tested it.
>
> Fine, I will remove that very commit comment.
>
> > I think the clock rate should be assigned in DTS and you should verify
> > it falls in the supported clock ranges.
>
> Which driver does that, please?
> clk_set_rate() appears to be used by multiple drivers, I was thinking
> I can use it.
> I can see clock frequencies in DTS, but they usually refer to fixed
> clocks (e.g. crystal-based). The clock I use is IMX6QDL_CLK_CKO, some
> sort of a shared generic clock from inside of the CPU. I don't know if
> I could even set its frequency in DTS.
>
> >> total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
> >> But this needs verifying.
> >
> > There's usually a minimum amount of blankings to be respected, not a
> > total, even less if the sensor capable of producing smaller modes
> > through subsampling. Is this sensor different ? I haven't found any
> > mention in the datasheet.
>
> I got these values experimentally. But it was few years ago and I have
> to look at them again.
>
> > Everyone has preferences. I do have mine, and they DON'T MATTER when I
> > submit code for inclusion.
>
> This is a straw man argument, I don't force anyone to my prefs (like
> e.g. 4-chars tabs). What we have here is a written coding-style
> document, and documented amendments from high profile developers.
> Now there are additional, informal and previously unknown to me
> requirements, of unknown source, which I'm apparently required to
> follow, so my questions shouldn't came as a surprise, should they?
>
You have never been told before, while submitting code to Linux, not
to use C++ comments ? Are you surprised someone contests that ?
> > "We" is me reporting what I've been told in these years from people
> > that spent their time reviewing my code and making sure the result is
> > consistent among the whole subsystem. The style of this driver is
> > totally alien to any convention in place here, and if you can ignore
> > my comments as I'm just a random idiot on the internet, you have been
> > told the same by the other people that cared enough to look into your
> > code.
>
> Well I must say I wasn't told that "style of this driver is totally
> alien to any convention in place here" by anyone else yet.
>
It is.
No driver in media (which I'm aware of) uses C++ comments.
Your one is entirely commented with C++ comments.
They all try to stay in the 80-cols limit.
Yours have lines that span to 140 cols and goes regularly over 100.
Lowercase hex is preferred (based on the review I read on the list)
but not enforced, I give you that.
> > It all really sounds like "it's better my way, please don't annoy me".
> > I don't find this a productive way to operate in a cooperative space
> > :)
>
> I have already changed a lot of stuff, based on the feedback by several
> people, and will probably change a lot more. Is it really like you say?
> I don't think so.
> Now swap the seats and tell me that what you say isn't now true.
>
> Think about it.
>
Ok, I give up then, feels like a waste of time reviewing a driver
(for the only sake of code consistency) and have every single comment
contested.
I don't have interest in blocking this driver, the contrary, if it's
fine for everyone else the way it is. Let me just point you to
Documentation/driver-api/media/maintainer-entry-profile.rst
and has there suggested have the patch go through
./scripts/checkpatch.pl --strict --max-line-length=80
Then I'll stop bothering about style and I'll let maintainers decide
what to pick up.
> >> Even on otherwise idle I2C bus this would add unnecessary latency.
> >> I prefer to have a much faster code, even if using that be() macro
> >> (which isn't ugly IMHO).
> >
> > 'be()' is bad as it is only used in this driver, modifying the
> > register tables requires to handle the be() macro. I had tested this
> > by moving register tables to and from a different driver and I had to
> > manually insert the be() thing and remove it on the other way around.
> >
> > For one that writes code there are tens that reads, use and modify it.
> > And you should be mostly concerned about them.
>
> I think I have to be most concerned about the operation of the driver.
> What good is a nicely looking code if it doesn't work?
> Also, I would think the author (me in this case) is the person who reads
> his/her code most often, though obviously it may be much less that all
> others combined.
>
> I'd love to get rid of the be(), though. What do you propose instead?
>
Mode based sensor drivers usually rely on long register tables, whose
writing is an expensive operation to be done at streamon time. Power
up is usually done at devnode open time but you relay on the legacy
s_power() here, so it's in control of the receiver driver which
depending on the implemenation might call it at open() time or stream
on. Sorry, I didn't notice that, has you register a devnode I assumed
you had an open() function, which you don't.
You have two tables of registers:
- the initial settings (70 register) that you write in pairs (which
means 280 bytes on the bus)
- recommended timings (216 registers) that you write in a single i2c
transaction. A third of those registers is 0x0000 (no be() is
needed).
There are more or less 15 other occurrences of be(), which are for
registers writes at streamon time (timings, pll and gains mostly).
The efficiency argument holds as long as we are in an hot path and I
understand writing 216 registers in pairs has an overhead which to
me, at open devnode open time is marginal, but if done at streamon time
should be avoided.
Please keep the suggested timings table declared as big endian in
place then. I would not use be() to write the handful of registers for
pll and gains at runtime, but up to you.
> > The long table writes should be done once at power-up time, not in any
> > hot path.
>
> Power-up isn't "once" anymore. It looks like it's to be done before
> every start of capture operation (my device will not do that).
>
As said, as your driver registers a subdev devnode, but also support
operations with a devnode-centric receivers, you're not in control of
when s_power() is called.
> > That said, if the efficiency argument holds for others, feel free to
> > keep it the way it is.
>
> It's just how the 1982 hardware thing work.
>
> > I tested with that yes, I can try remove the already programmed totals
> > and see if it's only 0x300c that makes a difference
>
> Just tested it and it works for me in 1920x1080p30 without any changes.
> Would it be possible it's the gain/exposure settings? If not, what exact
> clock frequency (for the chip) do you use?
24Mhz
> (I haven't yet a chance to test this 0x300c register, but will do).
I tested it again, and programming the right 0x300c (or the mirror
register) makes a difference between having completely dark images or
good ones.
Also I got mangled frames before I changed the streamon sequence to
what I've shared. But I need to verify that again as the receiver code
was moving too.
>
> > Do you know why registers are mirrored ? I haven't find any reference
> > to register shadowing in the manual.
>
> I think the first set follows some standard, but it lacks something -
> thus a new set. Or maybe it's how the chip was build from blocks - I
> don't know.
>
The difference is that the 0x3xxx ones are frame synchronized and
apply to 'bad frames' too.
> >> When you disable streaming, the clock lanes are apparently in LP-11
> >> state, however data lanes are LP-00.
> >
> > I see. Reading this and your below reply I get you need to enable test
> > mode, force the lines in LP-11 state and then disable the test mode
> > and start streaming. Feels like an ack, but maybe that's how the
> > sensor is intended to be operated.
>
> I don't think so. I think, in proprietary development, nobody cares
> about what does the chip send while not streaming.
>
afaict only imx6 has this check enforced (but I might be wrong)
> > I anyway think doing this here is not the best idea. "We" have a
> > pre_streamon() operation but it's also usually done at s_stream(0)
> > which you can call at device node open time to coax the lines in
> > LP-11. You already do most of these things, so I would move the test
> > mode handling to s_stream(0) and start/stop stream by just toggling
> > the RESET_STREAM bit as suggested in the s_stream() code snippet I've
> > shared in my previous reply.
>
> I don't get it completely. However I see I could use the pre_stream()
> to put the sensor in LP-11 mode (doing this in s_stream(0) would then be
> pointless). I will see if it works.
Be aware that relying on pre_streamon() requires the receiver driver
to be instrumented to call the operation. Doing that at s_stream(0)
time works with all receivers.
> RESET_STREAM must be already set to 1 (ACTIVE STREAMING) before
> s_stream(1) is called.
>
> >> > This register controls the HiSPI interface while my understanding is
> >> > that everything is MIPI CSI-2 in the rest of the driver. Why is it
> >> > here ? does it play any role in your setup ?
> >>
> >> Yes, it also controls MIPI mode.
> >
> > Maybe by accident, as a consequence enabling/disabling the test modes on
> > the CSI-2 lanes ?
>
> I guess it simply enables/disables the chosen test mode in the CSI-2
> lanes :-)
> I've chosen the test mode to be, well, LP-11 on all lines.
>
> >> >> + return ar0521_write_reg(sensor, AR0521_REG_RESET,
> >> >> + AR0521_REG_RESET_DEFAULTS |
> >> >> + AR0521_REG_RESET_RESTART |
> >> >> + AR0521_REG_RESET_STREAM);
> >> >> +}
> >>
> >> That's correct. It's the only way I have found.
> >
> > I think it's wrong. RESTART as per its definition interrupts the frame
> > and re starts the stream. START enables/disable streaming. You set
> > them both, even at s_stream(0) time, and things might work by
> > accident because you enable/disable the test mode at s_stream() time.
> >
> > As suggested I would rather coax the data lanes in LP-11 state at
> > s_stream(0) time by enabling test mode after having disabled streaming
> > by clearing the RESET_STREAM bit in AR0521_REG_RESET.
>
> How would you do that?
> If you disable streaming, LP-11 is gone.
> You need STREAMING to actually "stream" LP-11.
>
Even for test mode ? So for you streamoff is:
- Enable test mode (programmed to be LP-11)
- Start stream
?
Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?
> > Similarly at
> > s_stream(1) time I would disable the test mode (I bet it could be done
> > by clearing bits [9:6] in 0x3066 without touching the HiSpi control
> > register) and the start streaming by setting RESET_STREAM.
>
> You lost your bet :-)
> Clearing these bits disables test on data lanes only.
Ack
> To disable clock lane test you have to clear 0x31C6.
> This is what I do in s_stream(1):
>
> // normal output on clock and data lanes
> ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
>
> 0x31C6 = AR0521_REG_HISPI_CONTROL_STATUS which is a misnomer and perhaps
> I should call it AR0521_REG_MIPI_AND_HISPI_CONTROL_STATUS.
>
I don't have a way to test LP-11 state, but it feels really weird to
have to go through a test mode, programmed through a register
dedicated to HiSPi handling, to be able to have lanes in that state.
But sensors are weird, and if this works for you it's fine.
I would just try to centralize it in s_stream()
> >> I wonder if you can call set_fmt() etc. when the sensor is streaming.
> >> Why not?
> >
> > Because
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#c.V4L.VIDIOC_SUBDEV_S_FMT
> >
> > EBUSY
> > The format can’t be changed because the pad is currently busy. This
> > can be caused, for instance, by an active video stream on the pad. The
> > ioctl must not be retried without performing another action to fix the
> > problem first. Only returned by VIDIOC_SUBDEV_S_FMT
>
> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
> only that it MAY BE returned. Looking at the code, I can see nothing
> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
There's nothing in the core that has the notion of 'active streaming'
> v4l2_subdev_call(set_fmt)). It also appears several drivers update
> registers in their set_fmt(), so I think it's the desirable action
> there.
>
I hardly see a case where changing format on the sensor through an
operation on the subdev while streaming, is a good idea.
Also, you have a devnode, so you can change the format on the subdev
only, without informing the receiver.
Also, your s_fmt() and s_frame_interval() call write_mode() which
stops the streaming and it doesn't get restarted. I think that's
wrong and it's an undesired side effect.
(Also had a chat with Hans about this, the takeaway is that it's a
really bad idea and you need very strong reasons to allow that. It
could be considered for extreme cases like changing the color spaces of
reducing the image size as the allocated buffers are big enough but
again, you need very strong reasons to do so)
> >> >> + ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
> >> >
> >> > I wasn't able to interpret this register right
> >> >
> >> > pc_speed
> >> > Slows down the internal pixel clock frequency relative to the system
> >> > clock frequency. A programmed value of N gives a pixel clock period
> >> > of N system clocks. Only values 1, 2 and 4 are supported.
> >> >
> >> > Shouldn't this be part of the PLL calculation ?
> >>
> >> Well, this doesn't seem to be part of the PLL, and it's constant (as
> >> long as MIPI lane# is constant). Why do you think it belongs there?
> >
> > Because seems like a clock downscaler intended to be used to adapt the CSI-2
> > clock frequency (lane dependent) to the pixel clock and could be
> > programmed once.
>
> Exactly. This "programmed once" (per power-up) is exactly why I set it
> in ar0521_power_on() instead of calc_pll().
> First, calc_pll is calculations-only, no hw access (suggested by Laurent
> IIRC, and it plays well). Second, calc_pll() is called multiple times.
>
> > Also note that the number of data lanes in use can actually be modified at
> > run time but it's not something that has to be considered here.
>
> Yes, #lanes is constant here, though variable lane count could be
> useful - for tests, and in my other (unpublished) experiments. I don't
> know if it was possible back then, when I wrote the driver.
>
> [initial_regs]
>
> >> You know, I try not to throw unnecessary traffic at I2C bus, too.
> >
> > So this could moved to the register tables ? :)
>
> The truth is, I've considered doing exactly that. But they're quite
> fragmented (pixel_timing_recommended is linear) and well, perhaps I
> didn't want to complicate code too much. I may still do it, though.
> Guess I should.
>
> > i think you can remove any reference to events
>
> Good :-)
>
> > You should probably also support init_cfg as you expose a subdev video
> > device and you can have an init_format() or similar to be called there
> > for the try format and called here for the active format.
>
> Will have a look.
>
> > Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
> > disabled seems pointless. A minor anyway as it's in the driver's
> > remove function.
>
> Daniel already posted a comment about this, however the whole
> pm_runtime*() in this driver is a bit fragile and I'd welcome someone
> who know how does this PM stuff work looking at it...
> ... with the following quick patch applied on top of v5:
>
> - pm_runtime_idle(&client->dev);
> + if (pm_runtime_idle(&client->dev) == -ENOSYS) {
> + ret = ar0521_power_on(sensor);
> + if (ret)
> + goto disable;
> + }
> dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
> sensor->extclk_freq, sensor->lane_count);
> return 0;
>
> +disable:
> + v4l2_async_unregister_subdev(&sensor->sd);
> + media_entity_cleanup(&sensor->sd.entity);
> free_ctrls:
> v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> entity_cleanup:
>
> (Power is only needed after probe()).
>
> I'll post v6 eventually, of course, but have to squeeze this in
> somewhere.
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
Jacopo Mondi <[email protected]> writes:
> You have never been told before, while submitting code to Linux, not
> to use C++ comments ? Are you surprised someone contests that ?
Surprised, far from it. Linus has decided C++ comments are ok.
I simply follow his advice (C++ comments are, after all, technically
a little bit better).
> No driver in media (which I'm aware of) uses C++ comments.
> Your one is entirely commented with C++ comments.
>
> They all try to stay in the 80-cols limit.
> Yours have lines that span to 140 cols and goes regularly over 100.
Linus has already (in 2016 IIRC) said 80 cols are BS. This is BTW what
several people (me included) postulated long before. Would it make sense
to use this limit now?
If the above constitutes being "alien", well, don't worry about it.
> Ok, I give up then, feels like a waste of time reviewing a driver
> (for the only sake of code consistency) and have every single comment
> contested.
Please note a didn't contest your every comment. Actually, I have
contested only those... which I contested, by definition. I have
(perhaps too silently) accepted the rest.
> Documentation/driver-api/media/maintainer-entry-profile.rst
>
> and has there suggested have the patch go through
> ./scripts/checkpatch.pl --strict --max-line-length=80
You suggest I'm to fix this (.rst) file first? I think I can at least
try.
>> I'd love to get rid of the be(), though. What do you propose instead?
>
> Mode based sensor drivers usually rely on long register tables, whose
> writing is an expensive operation to be done at streamon time. Power
> up is usually done at devnode open time but you relay on the legacy
> s_power() here,
It's been suggested I get rid of it, and I'm going to do exactly this.
> so it's in control of the receiver driver which
> depending on the implemenation might call it at open() time or stream
> on. Sorry, I didn't notice that, has you register a devnode I assumed
> you had an open() function, which you don't.
Should I have one? Why? Are other drivers expected to have an open()?
Shouldn't I register a devnode?
> The efficiency argument holds as long as we are in an hot path and I
> understand writing 216 registers in pairs has an overhead which to
> me, at open devnode open time is marginal, but if done at streamon time
> should be avoided.
It *may*be* marginal in some cases, but it you have a single I^2 bus for
a bunch of devices, some of them e.g. MEMS, it may be as well critical.
>> Just tested it and it works for me in 1920x1080p30 without any changes.
>> Would it be possible it's the gain/exposure settings? If not, what exact
>> clock frequency (for the chip) do you use?
>
> 24Mhz
I will try to use that.
What SoC (or MIPI receiver) are you using?
> The difference is that the 0x3xxx ones are frame synchronized and
> apply to 'bad frames' too.
Is it stated in the docs?
>> I don't think so. I think, in proprietary development, nobody cares
>> about what does the chip send while not streaming.
>>
>
> afaict only imx6 has this check enforced (but I might be wrong)
Possibly only in the official tree (not the FSL/NXP).
>> How would you do that?
>> If you disable streaming, LP-11 is gone.
>> You need STREAMING to actually "stream" LP-11.
>
> Even for test mode ? So for you streamoff is:
> - Enable test mode (programmed to be LP-11)
> - Start stream
> ?
That's correct. LP-11 here *is* a test mode.
> Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?
Not sure. Why do you think so?
> I don't have a way to test LP-11 state,
On i.MX6 you can read a MIPI RX status register. IIRC the results may be
a bit unclear, though - using an oscilloscope removes any doubt.
>> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
>> only that it MAY BE returned. Looking at the code, I can see nothing
>> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
>
> There's nothing in the core that has the notion of 'active streaming'
Come on. It appears Linux (from top to the bottom) will accept set_fmt
while streaming. With certain (most?) drivers only, that's it. Not that
I actually tested it, but the v4l2 core code suggests it.
So I'm either to return -EBUSY, or - as others, probably most drivers -
update the registers. I can't just drop it on the floor, and let the
driver apply it on the next s_stream(1)... can I?
> I hardly see a case where changing format on the sensor through an
> operation on the subdev while streaming, is a good idea.
I'm not in control of this.
> Also, your s_fmt() and s_frame_interval() call write_mode() which
> stops the streaming and it doesn't get restarted. I think that's
> wrong and it's an undesired side effect.
That would be wrong indeed, but I can't see it in the code.
write_mode() stop streaming only momentarily, I can't avoid this.
s_frame_interval() returns -EBUSY if streaming (which I guess I should
remove). If not for the return, it wouldn't stop streaming either.
I will do some experiments, though.
> (Also had a chat with Hans about this, the takeaway is that it's a
> really bad idea and you need very strong reasons to allow that. It
> could be considered for extreme cases like changing the color spaces of
> reducing the image size as the allocated buffers are big enough but
> again, you need very strong reasons to do so)
Ah, buffers are a different story. You can't, for example, request
buffers which streaming etc. This is a completely different territory.
Now... I don't have buffers :-)
It's a MIPI sensor, the output is a bunch of LVDS lines.
Certain devices (like THC63LVD104C, an LVDS -> parallel receiver) simply
always stream (well, not in powerdown and not without incoming clock).
They don't even notice that some format has maybe changed.
A MIPI sensor is a bit smarter than that, but buffers - it's the
receiver's problem.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
HI Krzysztof
On Wed, Oct 13, 2021 at 02:55:54PM +0200, Krzysztof Hałasa wrote:
> Jacopo Mondi <[email protected]> writes:
>
> > You have never been told before, while submitting code to Linux, not
> > to use C++ comments ? Are you surprised someone contests that ?
>
> Surprised, far from it. Linus has decided C++ comments are ok.
> I simply follow his advice (C++ comments are, after all, technically
> a little bit better).
>
> > No driver in media (which I'm aware of) uses C++ comments.
> > Your one is entirely commented with C++ comments.
> >
> > They all try to stay in the 80-cols limit.
> > Yours have lines that span to 140 cols and goes regularly over 100.
>
> Linus has already (in 2016 IIRC) said 80 cols are BS. This is BTW what
> several people (me included) postulated long before. Would it make sense
> to use this limit now?
>
Apparently, for the subsystem maintainers, yes.
> If the above constitutes being "alien", well, don't worry about it.
>
> > Ok, I give up then, feels like a waste of time reviewing a driver
> > (for the only sake of code consistency) and have every single comment
> > contested.
>
> Please note a didn't contest your every comment. Actually, I have
> contested only those... which I contested, by definition. I have
> (perhaps too silently) accepted the rest.
>
> > Documentation/driver-api/media/maintainer-entry-profile.rst
> >
> > and has there suggested have the patch go through
> > ./scripts/checkpatch.pl --strict --max-line-length=80
>
> You suggest I'm to fix this (.rst) file first? I think I can at least
> try.
No, I suggest you stick to what has been there recorded not longer
than 2 years ago.
If you prefer to go the other way around and change the subsystem
profile first, please go ahead.
>
> >> I'd love to get rid of the be(), though. What do you propose instead?
> >
> > Mode based sensor drivers usually rely on long register tables, whose
> > writing is an expensive operation to be done at streamon time. Power
> > up is usually done at devnode open time but you relay on the legacy
> > s_power() here,
>
> It's been suggested I get rid of it, and I'm going to do exactly this.
>
> > so it's in control of the receiver driver which
> > depending on the implemenation might call it at open() time or stream
> > on. Sorry, I didn't notice that, has you register a devnode I assumed
> > you had an open() function, which you don't.
>
> Should I have one? Why? Are other drivers expected to have an open()?
If you register a devnode, userspace can open it.
To each open file handled is associated a try format, which should be
initialized to some meaningful default.
The same format should (or could) be used for your active format.
Userspace that makes use of the V4L2 subdev API won't be able to fully
operate on this driver in its current state.
> Shouldn't I register a devnode?
>
Depends on your use case and how much you care about supporting
complex use cases.
If you operate with a legacy implementation where everything goes
through the video device node, you don't care about the subdev
devnode.
If you operate in a media-controller environment, where userspace is
in control of configuring each component on the pipeline, you should
register a devnode.
> > The efficiency argument holds as long as we are in an hot path and I
> > understand writing 216 registers in pairs has an overhead which to
> > me, at open devnode open time is marginal, but if done at streamon time
> > should be avoided.
>
> It *may*be* marginal in some cases, but it you have a single I^2 bus for
> a bunch of devices, some of them e.g. MEMS, it may be as well critical.
>
> >> Just tested it and it works for me in 1920x1080p30 without any changes.
> >> Would it be possible it's the gain/exposure settings? If not, what exact
> >> clock frequency (for the chip) do you use?
> >
> > 24Mhz
>
> I will try to use that.
> What SoC (or MIPI receiver) are you using?
>
> > The difference is that the 0x3xxx ones are frame synchronized and
> > apply to 'bad frames' too.
>
> Is it stated in the docs?
>
Yes, in the right-most columns of the register tables.
> >> I don't think so. I think, in proprietary development, nobody cares
> >> about what does the chip send while not streaming.
> >>
> >
> > afaict only imx6 has this check enforced (but I might be wrong)
>
> Possibly only in the official tree (not the FSL/NXP).
>
> >> How would you do that?
> >> If you disable streaming, LP-11 is gone.
> >> You need STREAMING to actually "stream" LP-11.
> >
> > Even for test mode ? So for you streamoff is:
> > - Enable test mode (programmed to be LP-11)
> > - Start stream
> > ?
>
> That's correct. LP-11 here *is* a test mode.
>
> > Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?
>
> Not sure. Why do you think so?
>
Because it's documented as:
Setting this bit causes the sensor to truncate the current frame at
the end of the current row and start resetting (integrating) the first
row. The delay before the first valid frame is read out is equal to
the integration time.
and since you're moving from test mode to stream mode, there's no
frame integration going on.
> > I don't have a way to test LP-11 state,
>
> On i.MX6 you can read a MIPI RX status register. IIRC the results may be
> a bit unclear, though - using an oscilloscope removes any doubt.
>
> >> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
> >> only that it MAY BE returned. Looking at the code, I can see nothing
> >> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
> >
> > There's nothing in the core that has the notion of 'active streaming'
>
> Come on. It appears Linux (from top to the bottom) will accept set_fmt
What do you mean with "Linux from top to the bottom" ?
What I meant is that the core cannot prevent ioctls and subdev
operations to be called on the sensor while streaming, as the 'is
streaming' state is only kept in the driver and there's no state
keeping in the V4L2 core.
> while streaming. With certain (most?) drivers only, that's it. Not that
> I actually tested it, but the v4l2 core code suggests it.
spec says no.
reviewers say no.
maintainers say no.
But if you think you're right, please go ahead.
> So I'm either to return -EBUSY, or - as others, probably most drivers -
maybe they just assume they knew better when they got being told
not to do so during review.
> update the registers. I can't just drop it on the floor, and let the
> driver apply it on the next s_stream(1)... can I?
>
> > I hardly see a case where changing format on the sensor through an
> > operation on the subdev while streaming, is a good idea.
>
> I'm not in control of this.
>
As you're not in control of how your driver will be used once merged
That's why there are specs, reviews and collective knowledge that
helps enforce a consistent behavior.
> > Also, your s_fmt() and s_frame_interval() call write_mode() which
> > stops the streaming and it doesn't get restarted. I think that's
> > wrong and it's an undesired side effect.
>
> That would be wrong indeed, but I can't see it in the code.
> write_mode() stop streaming only momentarily, I can't avoid this.
Yes you can, by not allowing s_fmt while streaming.
Stopping and restarting stream in the sensor, through a subdev ioctl
behind the back of the rest of the capture pipeline is just calling
for troubles.
There's a potential point of failure in every single part of the
capture pipeline, from the on-going transfer in the SoC's DMA engines
to the CSI-2 receiver port.
Even without the stop/restart sequence, what happens if you increase
the frame size which is output from the sensor without re-negotiating
buffers or image formats ?
There's no single valid reason to allow such a use case, if not making
out of it a matter of principle like you're doing.
> s_frame_interval() returns -EBUSY if streaming (which I guess I should
> remove). If not for the return, it wouldn't stop streaming either.
>
> I will do some experiments, though.
>
> > (Also had a chat with Hans about this, the takeaway is that it's a
> > really bad idea and you need very strong reasons to allow that. It
> > could be considered for extreme cases like changing the color spaces of
> > reducing the image size as the allocated buffers are big enough but
> > again, you need very strong reasons to do so)
>
> Ah, buffers are a different story. You can't, for example, request
> buffers which streaming etc. This is a completely different territory.
>
> Now... I don't have buffers :-)
>
> It's a MIPI sensor, the output is a bunch of LVDS lines.
> Certain devices (like THC63LVD104C, an LVDS -> parallel receiver) simply
> always stream (well, not in powerdown and not without incoming clock).
> They don't even notice that some format has maybe changed.
> A MIPI sensor is a bit smarter than that, but buffers - it's the
> receiver's problem.
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
Jacopo,
> To each open file handled is associated a try format, which should be
> initialized to some meaningful default.
I will look at it then.
> If you operate with a legacy implementation where everything goes
> through the video device node, you don't care about the subdev
> devnode.
This can't be done in case of a sensor driver, right? Output from the
sensor is MIPI CSI, there is no connection to the memory bus.
BTW: What SoC (or MIPI receiver etc.) are you using?
>> > The difference is that the 0x3xxx ones are frame synchronized and
>> > apply to 'bad frames' too.
>>
>> Is it stated in the docs?
>
> Yes, in the right-most columns of the register tables.
Right.
Interesting - they are marked both "frame sync'd" and "bad frame" (not
all of them), where the 0x340 ones are both "N". I will try some tests,
but such a combination looks a bit suspicious.
(I guess I already tested it, but don't remember the results).
> Because it's documented as:
>
> Setting this bit causes the sensor to truncate the current frame at
> the end of the current row and start resetting (integrating) the first
> row. The delay before the first valid frame is read out is equal to
> the integration time.
>
> and since you're moving from test mode to stream mode, there's no
> frame integration going on.
But there is :-)
Remember we're streaming, the whole sensor is working. It's just the
LP-11 on the output lines.
> What do you mean with "Linux from top to the bottom" ?
Userspace + ioctl on top, the driver/hw on the bottom.
> What I meant is that the core cannot prevent ioctls and subdev
> operations to be called on the sensor while streaming, as the 'is
> streaming' state is only kept in the driver and there's no state
> keeping in the V4L2 core.
Exactly, that's what I thought.
>> while streaming. With certain (most?) drivers only, that's it. Not that
>> I actually tested it, but the v4l2 core code suggests it.
>
> spec says no.
I'm not aware of it. The specs say a driver *is*allowed* to return
EBUSY, if it can't handle the condition.
> reviewers say no.
Haven't seen this either. The existing code (other drivers) suggests
otherwise.
> maintainers say no.
Ditto.
Buffers are a completely different thing.
>> So I'm either to return -EBUSY, or - as others, probably most drivers -
>
> maybe they just assume they knew better when they got being told
> not to do so during review.
All of the others are wrong?
Maybe nobody told them otherwise - because there was no reason?
>> > I hardly see a case where changing format on the sensor through an
>> > operation on the subdev while streaming, is a good idea.
>>
>> I'm not in control of this.
>
> As you're not in control of how your driver will be used once merged
Exactly, it's the very same thing.
> That's why there are specs, reviews and collective knowledge that
> helps enforce a consistent behavior.
That's correct. Now you tell me I'm not to write to the hw in set_fmt(),
while I can clearly see other drivers do exactly that, and nobody else
suggests they (I) shouldn't.
> There's a potential point of failure in every single part of the
> capture pipeline, from the on-going transfer in the SoC's DMA engines
> to the CSI-2 receiver port.
I'm not in control of this, why should I set a policy for them? I don't
even know if there is a SoC and DMA engines - maybe this goes straight
to the antenna or *SDI transmitter? :-)
> Even without the stop/restart sequence, what happens if you increase
> the frame size which is output from the sensor without re-negotiating
> buffers or image formats ?
*I* don't change frame sizes. It's the upper levels which are making
such decisions. My code can only comply or return an error.
Perhaps the upper levels know what they're doing?
If they are wrong after all, well - a misprogrammed i.MX6 will corrupt
the frames, the output stream will lose sync, and the userspace can get
I/O errors on ioctls. The userspace will get what it asked for.
This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
effects will be exactly the same if the e.g. geometry changes come when
the sensor is not streaming.
> There's no single valid reason to allow such a use case, if not making
> out of it a matter of principle like you're doing.
Then why other drivers do exactly that? Eg. all imx*.
Including the newest one imx412, merged 2021-08-04, and:
Signed-off-by: Martina Krasteva <[email protected]>
Acked-by: Daniele Alessandrelli <[email protected]>
Acked-by: Paul J. Murphy <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Are they really all wrong? Really?
BTW principles are important to me, yes. One of them is "allow unless
required otherwise".
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
On Thu, Oct 14, 2021 at 07:43:02AM +0200, Krzysztof Hałasa wrote:
> Jacopo,
>
> > To each open file handled is associated a try format, which should be
> > initialized to some meaningful default.
>
> I will look at it then.
>
> > If you operate with a legacy implementation where everything goes
> > through the video device node, you don't care about the subdev
> > devnode.
>
> This can't be done in case of a sensor driver, right? Output from the
> sensor is MIPI CSI, there is no connection to the memory bus.
>
> BTW: What SoC (or MIPI receiver etc.) are you using?
>
> >> > The difference is that the 0x3xxx ones are frame synchronized and
> >> > apply to 'bad frames' too.
> >>
> >> Is it stated in the docs?
> >
> > Yes, in the right-most columns of the register tables.
>
> Right.
> Interesting - they are marked both "frame sync'd" and "bad frame" (not
> all of them), where the 0x340 ones are both "N". I will try some tests,
> but such a combination looks a bit suspicious.
> (I guess I already tested it, but don't remember the results).
>
> > Because it's documented as:
> >
> > Setting this bit causes the sensor to truncate the current frame at
> > the end of the current row and start resetting (integrating) the first
> > row. The delay before the first valid frame is read out is equal to
> > the integration time.
> >
> > and since you're moving from test mode to stream mode, there's no
> > frame integration going on.
>
> But there is :-)
> Remember we're streaming, the whole sensor is working. It's just the
> LP-11 on the output lines.
>
> > What do you mean with "Linux from top to the bottom" ?
>
> Userspace + ioctl on top, the driver/hw on the bottom.
>
> > What I meant is that the core cannot prevent ioctls and subdev
> > operations to be called on the sensor while streaming, as the 'is
> > streaming' state is only kept in the driver and there's no state
> > keeping in the V4L2 core.
>
> Exactly, that's what I thought.
>
> >> while streaming. With certain (most?) drivers only, that's it. Not that
> >> I actually tested it, but the v4l2 core code suggests it.
> >
> > spec says no.
>
> I'm not aware of it. The specs say a driver *is*allowed* to return
> EBUSY, if it can't handle the condition.
>
> > reviewers say no.
>
> Haven't seen this either. The existing code (other drivers) suggests
> otherwise.
>
> > maintainers say no.
>
> Ditto.
> Buffers are a completely different thing.
>
> >> So I'm either to return -EBUSY, or - as others, probably most drivers -
> >
> > maybe they just assume they knew better when they got being told
> > not to do so during review.
>
> All of the others are wrong?
> Maybe nobody told them otherwise - because there was no reason?
>
> >> > I hardly see a case where changing format on the sensor through an
> >> > operation on the subdev while streaming, is a good idea.
> >>
> >> I'm not in control of this.
> >
> > As you're not in control of how your driver will be used once merged
>
> Exactly, it's the very same thing.
>
> > That's why there are specs, reviews and collective knowledge that
> > helps enforce a consistent behavior.
>
> That's correct. Now you tell me I'm not to write to the hw in set_fmt(),
> while I can clearly see other drivers do exactly that, and nobody else
> suggests they (I) shouldn't.
>
> > There's a potential point of failure in every single part of the
> > capture pipeline, from the on-going transfer in the SoC's DMA engines
> > to the CSI-2 receiver port.
>
> I'm not in control of this, why should I set a policy for them? I don't
> even know if there is a SoC and DMA engines - maybe this goes straight
> to the antenna or *SDI transmitter? :-)
>
> > Even without the stop/restart sequence, what happens if you increase
> > the frame size which is output from the sensor without re-negotiating
> > buffers or image formats ?
>
> *I* don't change frame sizes. It's the upper levels which are making
> such decisions. My code can only comply or return an error.
> Perhaps the upper levels know what they're doing?
>
> If they are wrong after all, well - a misprogrammed i.MX6 will corrupt
> the frames, the output stream will lose sync, and the userspace can get
> I/O errors on ioctls. The userspace will get what it asked for.
>
> This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
> effects will be exactly the same if the e.g. geometry changes come when
> the sensor is not streaming.
>
No, this isn't true. Your s_fmt() implementation stops then restart the
stream. It has an undocumented side effect and will cause undefined
behaviour.
> > There's no single valid reason to allow such a use case, if not making
> > out of it a matter of principle like you're doing.
>
> Then why other drivers do exactly that? Eg. all imx*.
> Including the newest one imx412, merged 2021-08-04, and:
> Signed-off-by: Martina Krasteva <[email protected]>
> Acked-by: Daniele Alessandrelli <[email protected]>
> Acked-by: Paul J. Murphy <[email protected]>
> Signed-off-by: Sakari Ailus <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> Are they really all wrong? Really?
>
> BTW principles are important to me, yes. One of them is "allow unless
> required otherwise".
And that's how you will end up accepting the fact a s_fmt() has the
slightly disruptive side effect of stopping/restarting the stream
behind the back of the other components of the capture pipeline.
If your s_fmt() has to stop and restart streaming to take effect,
it means userspace should instead stop the stream, change
the format where opportune in the pipeline, and then restart the stream.
This allows the pipeline validation to take place, it allows buffers
to be negotiated correctly, it makes it impossible to write
application that rely on a side effect.
There's a lot going on about correctness and avoiding undefined
behaviours in the kernel these days. This is an example why I think a
language change won't solve much, not when it comes to correctness
towards user space. Allowing drivers to implement side-effects and
what clearly is an example of undefined behaviour is the recipe to
allow userspace to shot in their foot and makes it impossible to write
portable software with a predictable behaviour.
I still haven't heard a single reason why you want this, if not again,
for a matter of principles, which is by the way the less possible
productive way to carry on a converstation that should be based on
facts and reasons, not a fight to impose your mindset no
matter what.
Anyway, since we're not going anywhere here, I'll let this upscale to
anyone that will pick this driver up.
Thanks again for the contribution, I hope to see this driver in soon.
Cheers
j
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
Jacopo Mondi <[email protected]> writes:
>> This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
>> effects will be exactly the same if the e.g. geometry changes come when
>> the sensor is not streaming.
>>
>
> No, this isn't true. Your s_fmt() implementation stops then restart the
> stream. It has an undocumented side effect and will cause undefined
> behaviour.
It will cause *at*most* a corrupted frame. On a MIPI link. That's right.
Such a corrupted frame will *at*most* cause some transient IO error - it
must not cause anything serious, because corrupted frames on MIPI can
happen for multiple reasons, some of which simply cannot be avoided.
BTW I will see if it's actually the case - chances are, there is no
corruption, but I tested it years ago and haven't yet checked my notes.
In fact those set_fmt() in other drivers may - or may not - cause
corrupted frames just the same.
> If your s_fmt() has to stop and restart streaming to take effect,
> it means userspace should instead stop the stream, change
> the format where opportune in the pipeline, and then restart the
> stream.
Maybe. This is a sensor driver - not userspace.
If the userspace uses it as a part of "frame grabber", it will certainly
do exactly that (nothing else would make sense in practice).
Unfortunately this all will have to wait a bit, so thanks for your help,
expect a new patch in few weeks.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa