Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally <djrscally@gmail.com>]

Hello all

On 09/09/2021 23:36, Daniel Scally wrote:
> Hi Paul
>
> On 14/08/2021 21:56, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote:
>>> On 13/08/2021 04:05, Laurent Pinchart wrote:
>>>> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
>>>>> On 10/08/2021 15:29, Sakari Ailus wrote:
>>>>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
>>>>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>>>>>>>  				     0, 0, ov8865_test_pattern_menu);
>>>>>>>  
>>>>>>>  	/* Blanking */
>>>>>>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
>>>>>> Is the result in relation with the analogue crop size? Based on the above
>>>>>> it wouldn't seem like that.
>>>>> This was a weird one actually. My understanding was that HTS should
>>>>> always be >= the horizontal crop plus hblank...but that doesn't hold
>>>>> true for some of this driver's modes and nor does it hold true when
>>>>> running the camera in Windows (I checked the registers whilst running
>>>>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
>>>>> horizontal crop as the only way I could see to reconcile that.
>>>> There's something very fishy here, HTS is, by definition, equal to the
>>>> analog crop width plus the horizontal blanking. I suspect that the
>>>> values in ov8865_modes are wrong.
>>> I thought that initially too but confirming that the same thing happened
>>> running windows switched me into "you're probably wrong" mode. If we're
>>> confident that the HTS is likely wrong though I can add an extra patch
>>> to bring those into lining with that definition.
>> I think it's worth investigating this. The hblank computed here is
>> clearly incorrect, and would thus be useless for all practical purposes.
>> As usual with OmniVision, the datasheet is also quite useless.
>>
>> Paul, do you have any information about this ?
>
> A gentle ping on this...I played around setting HTS / VTS values whilst
> the camera was running windows; and it behaves as you'd expect it to
> (raising/lowering the frame rate), so as far as I can tell the sensor
> itself isn't doing anything unusual...


So, looking at this again. The mode in question has:

.output_size_x      = 3264
.hts                         = 1944
.output_size_y      = 2448
.vts                         = 2470
.frame_interval    = { 1, 30 }
   
And the driver sets a link frequency of 360MHz. That makes the pixel
rate, depending on whether we're looking at 2 or 4 data lanes, either
144MHz or 288MHz. I think the HTS there is calculated so that the 2 lane
configuration can make 30 FPS. Perhaps it would be better to default in
the mode to the "ideal" 4-lane 30fps setting (by upping the .hts to
3888), but rather than hardcoding the frame interval there, calculate it
for .g_frame_interval() based on the number of data lanes found in the
bus (accepting that if we only have 2 it's going to be 15fps rather than
30, which doesn't seem unreasonable for that resolution)