Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Mon, Nov 6, 2017 at 11:47 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 11/07/2017 07:04 AM, Tim Harvey wrote:
>> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey <tharvey@xxxxxxxxxxxxx> wrote:
>>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> <snip>
>>>>>
>>>>> Regarding video standard detection where this chip provides me with
>>>>> vertical-period, horizontal-period, and horizontal-pulse-width I
>>>>> should be able to detect the standard simply based off of
>>>>> vertical-period (framerate) and horizontal-period (line width
>>>>> including blanking) right? I wasn't sure if my method of matching
>>>>> these within 14% tolerance made sense. I will be removing the hsmatch
>>>>> logic from that as it seems the horizontal-pulse-width should be
>>>>> irrelevant.
>>>>
>>>> For proper video detection you ideally need:
>>>>
>>>> h/v sync size
>>>> h/v back/front porch size
>>>> h/v polarity
>>>> pixelclock (usually an approximation)
>>>>
>>>> The v4l2_find_dv_timings_cap() helper can help you find the corresponding
>>>> timings, allowing for pixelclock variation.
>>>>
>>>> That function assumes that the sync/back/frontporch values are all known.
>>>> But not all devices can actually discover those values. What can your
>>>> hardware detect? Can it tell front and backporch apart? Can it determine
>>>> the sync size?
>>>>
>>>> I've been considering for some time to improve that helper function to be
>>>> able to handle hardware that isn't able separate sync/back/frontporch values.
>>>>
>>>
>>> The TDA1997x provides only the vertical/horizontal periods and the
>>> horizontal pulse
>>> width (section 8.3.4 of datasheet [1]).
>>>
>>> Can you point me to a good primer on the relationship between these
>>> values and the h/v back/front porch?
>>>
>>> Currently I iterate over the list of known formats calculating hper
>>> (bt->pixelclock / V4L2_DV_BT_FRAME_WIDTH(bt)) and vper (hper /
>>> V4L2_DV_BT_FRAME_HEIGHT(bt)) (framerate) and find the closest match
>>> within +/- 7% tolerance. The list of supported formats is sorted by
>>> framerate then width.
>>>
>>>         /* look for matching timings */
>>>         for (i = 0; i < ARRAY_SIZE(tda1997x_hdmi_modes); i++) {
>>>                 const struct tda1997x_video_std *std = &tda1997x_hdmi_modes[i];
>>>                 const struct v4l2_bt_timings *bt = &std->timings.bt;
>>>                 int _hper, _vper, _hsper;
>>>                 int vmin, vmax, hmin, hmax, hsmin, hsmax;
>>>                 int vmatch, hsmatch;
>>>
>>>                 width = V4L2_DV_BT_FRAME_WIDTH(bt);
>>>                 lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
>>>
>>>                 _hper = (int)bt->pixelclock / (int)width;
>>>                 _vper = _hper / lines;
>>>                 _hsper = (int)bt->pixelclock / (int)bt->hsync;
>>>                 if (bt->interlaced)
>>>                         _vper *= 2;
>>>                 /* vper +/- 0.7% */
>>>                 vmin = 993 * (27000000 / _vper) / 1000;
>>>                 vmax = 1007 * (27000000 / _vper) / 1000;
>>>                 _hsper = (int)bt->pixelclock / (int)bt->hsync;
>>>                 if (bt->interlaced)
>>>                         _vper *= 2;
>>>                 /* vper +/- 0.7% */
>>>                 vmin = 993 * (27000000 / _vper) / 1000;
>>>                 vmax = 1007 * (27000000 / _vper) / 1000;
>>>                 /* hper +/- 0.7% */
>>>                 hmin = 993 * (27000000 / _hper) / 1000;
>>>                 hmax = 1007 * (27000000 / _hper) / 1000;
>>>
>>>                 /* vmatch matches the framerate */
>>>                 vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
>>>                 /* hmatch matches the width */
>>>                 hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
>>>                 if (vmatch && hsmatch) {
>>>                         v4l_info(state->client,
>>>                                  "resolution: %dx%d%c@%d (%d/%d/%d) %dMHz %d\n",
>>>                                  bt->width, bt->height, bt->interlaced?'i':'p',
>>>                                  _vper, vper, hper, hsper, pixrate, hsmatch);
>>>                         state->fps = (int)bt->pixelclock / (width * lines);
>>>                         state->std = std;
>>>                         return 0;
>>>                 }
>>>         }
>>>
>>> Note that I've thrown out any comparisons based on horizontal pulse
>>> width from my first patch as that didn't appear to fit well. So far
>>> the above works well however I do fail to recognize the following
>>> modes (using a Marshall SG4K HDMI test generator):
>>>
>>
>> Hans,
>>
>> I've found that I do indeed need to look at the 'hsper' that the TDA
>> provides above along with the vper/hper as there are several timings
>> that match a given vper/hper. However I haven't figured out how to
>> make sense of the hsper value that is returned.
>>
>> Here are some example timings and the vper/hper/hsper returned from the TDA:
>> V4L2_DV_BT_DMT_1280X960P60 449981/448/55
>> V4L2_DV_BT_DMT_1280X1024P60 449833/420/27
>> V4L2_DV_BT_DMT_1280X768P60 450021/568/11
>> V4L2_DV_BT_DMT_1360X768P60 449867/564/34
>>
>> Do you know what the hsper could be here? It doesn't appear to match
>> v4l2_bt_timings hsync ((27MHz/bt->pixelclock)*bt->hsync).
>
> Actually, all numbers except for the first match (assuming V4L2_DV_BT_DMT_1280X768P60
> is really V4L2_DV_BT_DMT_1280X768P60_RB).

Hans,

These are actual timings that I'm feeding the TDA input from a
Marshall V-SG4K-HDI HDMI test generator so they really are different
timings and those are the vper/hper/hsper the TDA reports for them.
Also, when I look at the original vendor code which had pre-calculated
min/max numbers for vper/hper/hsper it matches the numbers above so I
don't think the HDMI test generator is in error.

Tim

>
> Are you sure about the first one?
>
> Unfortunately, due to rounding errors the hsper is simply not accurate enough to
> use reliably. Furthermore, what is really needed if you want to add support for
> GTF and CVT standards is the vsync value, and that's not reported at all.
>
> I'd just give up on this and use your original code.
>
> Very poor hardware design :-(
>
> Regards,
>
>         Hans