Hi Dan, On 1/29/24 10:53, Dan Scally wrote: > Morning Hans > > > On 28/01/2024 20:58, Hans de Goede wrote: >> Hi All, >> >> While adding vblank ctrl support to the ov2680 driver I was looking >> at the vblank ctrl code in the ov5693 and I noticed something >> which I believe is weird / undesirable. >> >> When setting a new mode the ov5693 driver does not keep the current >> vts value (adjusting vblank to keep vts and thus the framerare and >> exposure unchanged). >> >> Instead it calculates a new vts value, resetting the framerate >> to 30 fps; or 60 fps for smaller resolutions and then sets >> vblank to the new (vts - new_mode->height) and adjusts >> the exposure control-range to fit within the new vts, potentially >> also changing the exposure value. >> >> This behavior of the ov5693 code means that switching resolution >> also changes the framerate and the exposure value which seems >> undesirable. > > > I think I did it that way because I was hitting problems when changing the framesize exceeded the current VTS and it seemed cleaner to just reset it to a known situation. Really though the only thing it would affect would be the framerate; that would have to reduce if the VTS increased but exposure could stay the same (though the maximum would change). So probably it ought to work like: > > > * if we change from a larger to a smaller framesize then we can just increase blanking to keep the same VTS and that should be fine > > * if we're going from a smaller to a larger framesize that fits within the currently configured VTS with minimum blanking, we can just reduce the blanking to keep the same VTS and that should be fine > > * if we're going from a smaller to a larger framesize that exceeds the currently configured VTS we drop blanking to a minimum so that the new framerate is as close to the old one as it can be > > > Does that sound like more reasonable behaviour? If so, shall I patch it? This sounds more or like what I had in mind (keep VTS unchanged if possible), so I have been looking more into this yesterday evening and implementing this is a bit tricky (*). Combining this with your last point of "that the new framerate is as close to the old one as it can be" I think I prefer a more KISS approach. IMHO the best thing (principle of least surprise) would be to on a set_fmt pad-op reset things to a default fps of say 30, as Jacopo's doc patches suggest. My reasons for suggesting this approach is: a) This is easier to implement and thus less likely to have bugs b) It leads to consistent behavior where as your suggested try to keep current vts approach leads to sometimes vts being kept, but other times when going from smaller to higher resolutions vts changing which will lead to hard to debug problems if userspace relies on vts being kept. For the ov5693 driver this would mean dropping __ov5693_calc_vts() replacing it with a DEFAULT_VTS define of: ALIGN_DOWN(OV5693_PIXEL_RATE / OV5693_FIXED_PPL / 30, 2) (does vts need to be a multiple of 2? We don't enforce that in the vblank control) Regards, Hans p.s. What about enum/get/set frame_interval vs set_mode vs vblank ctrl ? Options: a) Sensor drivers MUST not implement enum/get/set frame_interval? b) enum/get/set frame_interval only enum/get/set the default frame_interval set by set_mode (e.g. fixed 30 fps). Any vblank changes made after the set_mode are not reflected by get_frame_interval and set_frame_interval only influences the next set_mode call, not the current mode ? Or maybe only allow set_frame_interval when not streaming and then have it set vblank to match the interval like it would have done when called before the last set_mode call ? c) enum/get/set frame_interval are a second way to control hts (lets not go there just here for completeness sake) My own preference here would be to go with a) . *) As Jacopo's doc patches mention the vblank range needs to be updated when changing the mode. Which means calling __v4l2_ctrl_modify_range() now this will clamp vblank to the new range, potentially changing it leading to a __v4l2_ctrl_s_ctrl() call under the hood. We need to do this __v4l2_ctrl_modify_range() before actually calling __v4l2_ctrl_s_ctrl() to set the new vblank value (the new value calculated to keep vts the same). Otherwise the new value may be out of range and we must not directly poke the v4l2-ctrl internals to set a new in range value before calling __v4l2_ctrl_modify_range(). So this lead to multiple control-change events being emitted to userspace. But this is unavoidable even with more KISS approaches. Also when vts changes we also need to ensure that the exposure range is corrected. Theoretically it is possible for vblank to stay unchanged (e.g. changed from minimum vblank to minimum vblank) so we cannot rely on s_ctrl to update the exposure range. Note updating the exposure range twice is not a big deal since __v4l2_ctrl_modify_range() checks if things actually change and otherwise it is a no-op. > > > Thanks > > Dan > >> >> The vblank and hblank control values changes on setting a mode >> is unavoidable but the framerate and exposure value changing >> at the same time seems undesirable. >> >> Note that this also halves the max supported exposure value >> when going to a lower-res mode even when userspace never >> touches the vblank control. >> >> Regards, >> >> Hans >> >> >> >> >> >
