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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> >>>> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
> >>>> +					  hblank, 1, hblank);
> >>>> +
> >>>> +	if (ctrls->hblank)
> >>>> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >>>> +
> >>>>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
> >>>>  	vblank_def = mode->vts - mode->output_size_y;
> >>>>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

-- 
Regards,

Laurent Pinchart