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  wrote:
> On 11/07/2017 07:04 AM, Tim Harvey wrote:
>> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
>>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
>>> 
>
> 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 = 
>>> _hdmi_modes[i];
>>> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
>>> vmax = 1007 * (2700 / _vper) / 1000;
>>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>>> if (bt->interlaced)
>>> _vper *= 2;
>>> /* vper +/- 0.7% */
>>> vmin = 993 * (2700 / _vper) / 1000;
>>> vmax = 1007 * (2700 / _vper) / 1000;
>>> /* hper +/- 0.7% */
>>> hmin = 993 * (2700 / _hper) / 1000;
>>> hmax = 1007 * (2700 / _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:
>> 

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  wrote:
> On 11/07/2017 07:04 AM, Tim Harvey wrote:
>> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
>>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
>>> 
>
> 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 = 
>>> _hdmi_modes[i];
>>> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
>>> vmax = 1007 * (2700 / _vper) / 1000;
>>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>>> if (bt->interlaced)
>>> _vper *= 2;
>>> /* vper +/- 0.7% */
>>> vmin = 993 * (2700 / _vper) / 1000;
>>> vmax = 1007 * (2700 / _vper) / 1000;
>>> /* hper +/- 0.7% */
>>> hmin = 993 * (2700 / _hper) / 1000;
>>> hmax = 1007 * (2700 / _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 

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

[Hans Verkuil]

On 11/07/2017 07:04 AM, Tim Harvey wrote:
> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
>> 

 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 = 
>> _hdmi_modes[i];
>> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>> if (bt->interlaced)
>> _vper *= 2;
>> /* vper +/- 0.7% */
>> vmin = 993 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> /* hper +/- 0.7% */
>> hmin = 993 * (2700 / _hper) / 1000;
>> hmax = 1007 * (2700 / _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 

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

[Hans Verkuil]

On 11/07/2017 07:04 AM, Tim Harvey wrote:
> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
>> 

 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 = 
>> _hdmi_modes[i];
>> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>> if (bt->interlaced)
>> _vper *= 2;
>> /* vper +/- 0.7% */
>> vmin = 993 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> /* hper +/- 0.7% */
>> hmin = 993 * (2700 / _hper) / 1000;
>> hmax = 1007 * (2700 / _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 

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

[Tim Harvey]

On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
> 
>>>
>>> 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 = 
> _hdmi_modes[i];
> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> _hsper = (int)bt->pixelclock / (int)bt->hsync;
> if (bt->interlaced)
> _vper *= 2;
> /* vper +/- 0.7% */
> vmin = 993 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> /* hper +/- 0.7% */
> hmin = 993 * (2700 / _hper) / 1000;
> hmax = 1007 * (2700 / _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).

Thanks,

Tim

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

[Tim Harvey]

On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
> 
>>>
>>> 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 = 
> _hdmi_modes[i];
> const struct v4l2_bt_timings *bt = >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 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> _hsper = (int)bt->pixelclock / (int)bt->hsync;
> if (bt->interlaced)
> _vper *= 2;
> /* vper +/- 0.7% */
> vmin = 993 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> /* hper +/- 0.7% */
> hmin = 993 * (2700 / _hper) / 1000;
> hmax = 1007 * (2700 / _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).

Thanks,

Tim

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

[Hans Verkuil]

On 11/04/2017 01:17 AM, Tim Harvey wrote:
> On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>>
>> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>>

 I see the AVI infoframe has hdmi_quantization_range and
 hdmi_ycc_quantization_range along with vid_code.

 I'm not at all clear what to do with this information. Is there
 anything you see in the datasheet [1] that points to something I need
 to be doing?
>>>
>>> You can ignore hdmi_ycc_quantization_range, it is the 
>>> hdmi_quantization_range
>>> that you need to read out.
>>>
>>> The TDA can receive the following formats:
>>>
>>> RGB Full Range
>>> RGB Limited Range
>>> YUV Bt.601 (aka SMPTE 170M)
>>> YUV Rec.709
>>>
>>> The YUV formats are always limited range.
>>>
>>> The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
>>> and
>>> YUV to be limited range. YUV can be either 601 or 709.
>>>
>>> So if the TDA transmits RGB then you need to support the following 
>>> conversions:
>>>
>>> RGB Full -> RGB Full
>>> RGB Limited -> RGB Full
>>> YUV 601 -> RGB Full
>>> YUV 709 -> RGB Full
>>>
>>> And if the TDA transmits YUV then you need these conversions:
>>>
>>> RGB Full -> YUV601 or YUV709
>>> RGB Limited -> YUV601 or YUV709
>>> YUV601 -> YUV601
>>> YUV709 -> YUV709
>>>
>>> For the RGB to YUV conversion you have a choice of converting to YUV601 or 
>>> 709.
>>> I recommend to either always convert to YUV601 or to let it depend on the 
>>> resolution
>>> (SDTV YUV601, HDTV YUV709).
>>>
>>
>> Ok - this is a good explanation that I should be able to follow. I
>> will make sure to take into account hdmi_quantization_range when I
>> setup the colorspace conversion matrix for v3.
> 
> Hans,
> 
> I'm having trouble figuring out the conversion matrix to use between
> limited and full.
> 
> Currently I have the following conversion matrices, the values which
> came from some old vendor code:
> 
> /* Colorspace conversion matrix coefficients and offsets */
> struct color_matrix_coefs {
> /* Input offsets */
> s16 offint1;
> s16 offint2;
> s16 offint3;
> /* Coeficients */
> s16 p11coef;
> s16 p12coef;
> s16 p13coef;
> s16 p21coef;
> s16 p22coef;
> s16 p23coef;
> s16 p31coef;
> s16 p32coef;
> s16 p33coef;
> /* Output offsets */
> s16 offout1;
> s16 offout2;
> s16 offout3;
> };
> /* Conversion matrixes */
> enum {
> ITU709_RGBLIMITED,
> ITU601_RGBLIMITED,
> RGBLIMITED_ITU601,
>};
>static const struct color_matrix_coefs conv_matrix[] = {
> /* ITU709 -> RGBLimited */
> {
> -256, -2048,  -2048,
> 4096, -1875,   -750,
> 4096,  6307,  0,
> 4096, 0,   7431,
>  256,   256,256,
> },
> /* YUV601 limited -> RGB limited */
> {
> -256, -2048,  -2048,
> 4096, -2860,  -1378,
> 4096,  5615,  0,
> 4096, 0,   7097,
> 256,256,256,
> },
> /* RGB limited -> ITU601 */
> {
> -256,  -256,   -256,
> 2404,  1225,467,
> -1754, 2095,   -341,
> -1388, -707,   2095,
> 256,   2048,   2048,
> },
> };
> 
> Assuming the above are correct this leaves me missing RGB limitted ->
> RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
> YUV601.
> 
> I don't have documentation for the registers but I'm assuming the
> input offset is applied first, then the multiplication by the coef,
> then the output offset is applied. I'm looking over
> https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
> but I'm unable to figure out how to apply those to the above. Any
> ideas?

For the YUV to RGB full conversions all you need to do is to change
the last row to 0, 0, 0 (since you no longer apply an offset) and
multiply all matrix coefficients by (255 / 219) to ensure the matrix
result is in the range [0-255] instead of [0-219].

For the RGB lim to RGB full conversion you need this matrix:

-256, -256, -256,
S, 0, 0,
0, S, 0,
0, 0, S,
0, 0, 0

Where S = 4096 * 255 / 219. I'm assuming 4096 equals 1.0 using fixed
point format.

Actually, looking at the order of the matrix values I suspect it might
be:

-256, -256, -256,
0, S, 0,
0, 0, S,
S, 0, 0,
0, 0, 0

You'll have to 

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

[Hans Verkuil]

On 11/04/2017 01:17 AM, Tim Harvey wrote:
> On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>>
>> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>>

 I see the AVI infoframe has hdmi_quantization_range and
 hdmi_ycc_quantization_range along with vid_code.

 I'm not at all clear what to do with this information. Is there
 anything you see in the datasheet [1] that points to something I need
 to be doing?
>>>
>>> You can ignore hdmi_ycc_quantization_range, it is the 
>>> hdmi_quantization_range
>>> that you need to read out.
>>>
>>> The TDA can receive the following formats:
>>>
>>> RGB Full Range
>>> RGB Limited Range
>>> YUV Bt.601 (aka SMPTE 170M)
>>> YUV Rec.709
>>>
>>> The YUV formats are always limited range.
>>>
>>> The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
>>> and
>>> YUV to be limited range. YUV can be either 601 or 709.
>>>
>>> So if the TDA transmits RGB then you need to support the following 
>>> conversions:
>>>
>>> RGB Full -> RGB Full
>>> RGB Limited -> RGB Full
>>> YUV 601 -> RGB Full
>>> YUV 709 -> RGB Full
>>>
>>> And if the TDA transmits YUV then you need these conversions:
>>>
>>> RGB Full -> YUV601 or YUV709
>>> RGB Limited -> YUV601 or YUV709
>>> YUV601 -> YUV601
>>> YUV709 -> YUV709
>>>
>>> For the RGB to YUV conversion you have a choice of converting to YUV601 or 
>>> 709.
>>> I recommend to either always convert to YUV601 or to let it depend on the 
>>> resolution
>>> (SDTV YUV601, HDTV YUV709).
>>>
>>
>> Ok - this is a good explanation that I should be able to follow. I
>> will make sure to take into account hdmi_quantization_range when I
>> setup the colorspace conversion matrix for v3.
> 
> Hans,
> 
> I'm having trouble figuring out the conversion matrix to use between
> limited and full.
> 
> Currently I have the following conversion matrices, the values which
> came from some old vendor code:
> 
> /* Colorspace conversion matrix coefficients and offsets */
> struct color_matrix_coefs {
> /* Input offsets */
> s16 offint1;
> s16 offint2;
> s16 offint3;
> /* Coeficients */
> s16 p11coef;
> s16 p12coef;
> s16 p13coef;
> s16 p21coef;
> s16 p22coef;
> s16 p23coef;
> s16 p31coef;
> s16 p32coef;
> s16 p33coef;
> /* Output offsets */
> s16 offout1;
> s16 offout2;
> s16 offout3;
> };
> /* Conversion matrixes */
> enum {
> ITU709_RGBLIMITED,
> ITU601_RGBLIMITED,
> RGBLIMITED_ITU601,
>};
>static const struct color_matrix_coefs conv_matrix[] = {
> /* ITU709 -> RGBLimited */
> {
> -256, -2048,  -2048,
> 4096, -1875,   -750,
> 4096,  6307,  0,
> 4096, 0,   7431,
>  256,   256,256,
> },
> /* YUV601 limited -> RGB limited */
> {
> -256, -2048,  -2048,
> 4096, -2860,  -1378,
> 4096,  5615,  0,
> 4096, 0,   7097,
> 256,256,256,
> },
> /* RGB limited -> ITU601 */
> {
> -256,  -256,   -256,
> 2404,  1225,467,
> -1754, 2095,   -341,
> -1388, -707,   2095,
> 256,   2048,   2048,
> },
> };
> 
> Assuming the above are correct this leaves me missing RGB limitted ->
> RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
> YUV601.
> 
> I don't have documentation for the registers but I'm assuming the
> input offset is applied first, then the multiplication by the coef,
> then the output offset is applied. I'm looking over
> https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
> but I'm unable to figure out how to apply those to the above. Any
> ideas?

For the YUV to RGB full conversions all you need to do is to change
the last row to 0, 0, 0 (since you no longer apply an offset) and
multiply all matrix coefficients by (255 / 219) to ensure the matrix
result is in the range [0-255] instead of [0-219].

For the RGB lim to RGB full conversion you need this matrix:

-256, -256, -256,
S, 0, 0,
0, S, 0,
0, 0, S,
0, 0, 0

Where S = 4096 * 255 / 219. I'm assuming 4096 equals 1.0 using fixed
point format.

Actually, looking at the order of the matrix values I suspect it might
be:

-256, -256, -256,
0, S, 0,
0, 0, S,
S, 0, 0,
0, 0, 0

You'll have to test this to verify which of the two is the 

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

[Tim Harvey]

On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>
> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>
> >>
> >> I see the AVI infoframe has hdmi_quantization_range and
> >> hdmi_ycc_quantization_range along with vid_code.
> >>
> >> I'm not at all clear what to do with this information. Is there
> >> anything you see in the datasheet [1] that points to something I need
> >> to be doing?
> >
> > You can ignore hdmi_ycc_quantization_range, it is the 
> > hdmi_quantization_range
> > that you need to read out.
> >
> > The TDA can receive the following formats:
> >
> > RGB Full Range
> > RGB Limited Range
> > YUV Bt.601 (aka SMPTE 170M)
> > YUV Rec.709
> >
> > The YUV formats are always limited range.
> >
> > The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
> > and
> > YUV to be limited range. YUV can be either 601 or 709.
> >
> > So if the TDA transmits RGB then you need to support the following 
> > conversions:
> >
> > RGB Full -> RGB Full
> > RGB Limited -> RGB Full
> > YUV 601 -> RGB Full
> > YUV 709 -> RGB Full
> >
> > And if the TDA transmits YUV then you need these conversions:
> >
> > RGB Full -> YUV601 or YUV709
> > RGB Limited -> YUV601 or YUV709
> > YUV601 -> YUV601
> > YUV709 -> YUV709
> >
> > For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> > 709.
> > I recommend to either always convert to YUV601 or to let it depend on the 
> > resolution
> > (SDTV YUV601, HDTV YUV709).
> >
>
> Ok - this is a good explanation that I should be able to follow. I
> will make sure to take into account hdmi_quantization_range when I
> setup the colorspace conversion matrix for v3.

Hans,

I'm having trouble figuring out the conversion matrix to use between
limited and full.

Currently I have the following conversion matrices, the values which
came from some old vendor code:

/* Colorspace conversion matrix coefficients and offsets */
struct color_matrix_coefs {
/* Input offsets */
s16 offint1;
s16 offint2;
s16 offint3;
/* Coeficients */
s16 p11coef;
s16 p12coef;
s16 p13coef;
s16 p21coef;
s16 p22coef;
s16 p23coef;
s16 p31coef;
s16 p32coef;
s16 p33coef;
/* Output offsets */
s16 offout1;
s16 offout2;
s16 offout3;
};
/* Conversion matrixes */
enum {
ITU709_RGBLIMITED,
ITU601_RGBLIMITED,
RGBLIMITED_ITU601,
   };
   static const struct color_matrix_coefs conv_matrix[] = {
/* ITU709 -> RGBLimited */
{
-256, -2048,  -2048,
4096, -1875,   -750,
4096,  6307,  0,
4096, 0,   7431,
 256,   256,256,
},
/* YUV601 limited -> RGB limited */
{
-256, -2048,  -2048,
4096, -2860,  -1378,
4096,  5615,  0,
4096, 0,   7097,
256,256,256,
},
/* RGB limited -> ITU601 */
{
-256,  -256,   -256,
2404,  1225,467,
-1754, 2095,   -341,
-1388, -707,   2095,
256,   2048,   2048,
},
};

Assuming the above are correct this leaves me missing RGB limitted ->
RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
YUV601.

I don't have documentation for the registers but I'm assuming the
input offset is applied first, then the multiplication by the coef,
then the output offset is applied. I'm looking over
https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
but I'm unable to figure out how to apply those to the above. Any
ideas?

Thanks,

Tim

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

[Tim Harvey]

On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>
> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>
> >>
> >> I see the AVI infoframe has hdmi_quantization_range and
> >> hdmi_ycc_quantization_range along with vid_code.
> >>
> >> I'm not at all clear what to do with this information. Is there
> >> anything you see in the datasheet [1] that points to something I need
> >> to be doing?
> >
> > You can ignore hdmi_ycc_quantization_range, it is the 
> > hdmi_quantization_range
> > that you need to read out.
> >
> > The TDA can receive the following formats:
> >
> > RGB Full Range
> > RGB Limited Range
> > YUV Bt.601 (aka SMPTE 170M)
> > YUV Rec.709
> >
> > The YUV formats are always limited range.
> >
> > The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
> > and
> > YUV to be limited range. YUV can be either 601 or 709.
> >
> > So if the TDA transmits RGB then you need to support the following 
> > conversions:
> >
> > RGB Full -> RGB Full
> > RGB Limited -> RGB Full
> > YUV 601 -> RGB Full
> > YUV 709 -> RGB Full
> >
> > And if the TDA transmits YUV then you need these conversions:
> >
> > RGB Full -> YUV601 or YUV709
> > RGB Limited -> YUV601 or YUV709
> > YUV601 -> YUV601
> > YUV709 -> YUV709
> >
> > For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> > 709.
> > I recommend to either always convert to YUV601 or to let it depend on the 
> > resolution
> > (SDTV YUV601, HDTV YUV709).
> >
>
> Ok - this is a good explanation that I should be able to follow. I
> will make sure to take into account hdmi_quantization_range when I
> setup the colorspace conversion matrix for v3.

Hans,

I'm having trouble figuring out the conversion matrix to use between
limited and full.

Currently I have the following conversion matrices, the values which
came from some old vendor code:

/* Colorspace conversion matrix coefficients and offsets */
struct color_matrix_coefs {
/* Input offsets */
s16 offint1;
s16 offint2;
s16 offint3;
/* Coeficients */
s16 p11coef;
s16 p12coef;
s16 p13coef;
s16 p21coef;
s16 p22coef;
s16 p23coef;
s16 p31coef;
s16 p32coef;
s16 p33coef;
/* Output offsets */
s16 offout1;
s16 offout2;
s16 offout3;
};
/* Conversion matrixes */
enum {
ITU709_RGBLIMITED,
ITU601_RGBLIMITED,
RGBLIMITED_ITU601,
   };
   static const struct color_matrix_coefs conv_matrix[] = {
/* ITU709 -> RGBLimited */
{
-256, -2048,  -2048,
4096, -1875,   -750,
4096,  6307,  0,
4096, 0,   7431,
 256,   256,256,
},
/* YUV601 limited -> RGB limited */
{
-256, -2048,  -2048,
4096, -2860,  -1378,
4096,  5615,  0,
4096, 0,   7097,
256,256,256,
},
/* RGB limited -> ITU601 */
{
-256,  -256,   -256,
2404,  1225,467,
-1754, 2095,   -341,
-1388, -707,   2095,
256,   2048,   2048,
},
};

Assuming the above are correct this leaves me missing RGB limitted ->
RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
YUV601.

I don't have documentation for the registers but I'm assuming the
input offset is applied first, then the multiplication by the coef,
then the output offset is applied. I'm looking over
https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
but I'm unable to figure out how to apply those to the above. Any
ideas?

Thanks,

Tim

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

[Tim Harvey]

On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:

>>
>> I see the AVI infoframe has hdmi_quantization_range and
>> hdmi_ycc_quantization_range along with vid_code.
>>
>> I'm not at all clear what to do with this information. Is there
>> anything you see in the datasheet [1] that points to something I need
>> to be doing?
>
> You can ignore hdmi_ycc_quantization_range, it is the hdmi_quantization_range
> that you need to read out.
>
> The TDA can receive the following formats:
>
> RGB Full Range
> RGB Limited Range
> YUV Bt.601 (aka SMPTE 170M)
> YUV Rec.709
>
> The YUV formats are always limited range.
>
> The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range and
> YUV to be limited range. YUV can be either 601 or 709.
>
> So if the TDA transmits RGB then you need to support the following 
> conversions:
>
> RGB Full -> RGB Full
> RGB Limited -> RGB Full
> YUV 601 -> RGB Full
> YUV 709 -> RGB Full
>
> And if the TDA transmits YUV then you need these conversions:
>
> RGB Full -> YUV601 or YUV709
> RGB Limited -> YUV601 or YUV709
> YUV601 -> YUV601
> YUV709 -> YUV709
>
> For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> 709.
> I recommend to either always convert to YUV601 or to let it depend on the 
> resolution
> (SDTV YUV601, HDTV YUV709).
>

Ok - this is a good explanation that I should be able to follow. I
will make sure to take into account hdmi_quantization_range when I
setup the colorspace conversion matrix for v3.

> Ideally the application should specify what it wants, but we don't have any 
> API
> support for that.
>

>>
>> 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?
>
> The blanking consists of a front porch width, a sync width and a back porch 
> width.
> 'Width' is normally measured in pixels. Vertical blanking is the same, except 
> that
> is measured in lines. All these values are defined in the standards that 
> define these
> timings (e.g. VESA DMT, CTA-861).
>
> So for 1080p60 the active video is 1920x1080. After each line of 1920 pixels 
> you
> have 88 'pixels' of front porch, a sync pulse of 44 'pixels' and a back porch 
> of
> 148 pixels. Total frame width is 2200. Similar for the vertical.
>
> A good HDMI receiver will give you the exact values for these blanking sizes.
> Especially the sync width/height is important since that can provide 
> additional
> format information when receiving VESA formats.
>
> It looks as if the TDA does not measure in exact pixels but in 27 MHz clock
> periods. Which is an approximation only.
>
> So it appears that what you do is the best you can do.
>
> Although I wonder about the hsper: the datasheet suggests that this is the 
> width,
> not a period. What is the value you read out when you receive 1080p60? If that
> would be an exact width, then that would help a lot since you can compare that
> against bt->hsync.

Here's a list of source modes and the vertical/horizontal period and
horizontal pulse width returned from the TDA:
00: VESA640x480P_60HZ 450427 856 101
01: VESA800x600P_60HZ 447620 711 85
02: VESA1024x768P_60HZ 449952 557 55
03: VESA1280x768P_60HZ 450021 568 11
04: VESA1360x768P_60HZ 449867 564 34
05: VESA1280x960P_60HZ 449981 448 55
06: VESA1280x1024P_60HZ 449833 420 27
07: VESA1400x1050P_60HZ 450372 416 7
08: VESA1600x1200P_60HZ 449981 358 27
09: VESA1920x1200P_60HZ 450355 363 4
10: CEAVIC1440x480I_60HZ 450430 1714 123
11: CEAVIC720x480P_60HZ 450430 856 61
12: CEAVIC1280x720P_60HZ 449981 598 13
13: CEAVIC1280x720P_59.94 450431 599 13
14: CEAVIC1920x1080I_60HZ 449981 798 15
15: CEAVIC1920x1080I_59.95HZ 450431 799 15
16: CEAVIC1920x1080P_30HZ 899962 798 15
17: CEAVIC1920x1080P_29.95HZ 900862 799 15
18: CEAVIC1920x1080P_24HZ 1124953 998 15
19: CEAVIC1920x1080P_23.976HZ 1126077 999 15
20: CEAVIC1920x1080P_60HZ 449981 398 7
21: CEAVIC1920x1080P_59.94HZ 450431 399 7
22: CEAVIC1440x576I_50HZ 539976 1726 127
23: CEAVIC720x576P_50HZ 539976 862 63
24: CEAVIC1280x720P_50HZ 539977 718 13
25: CEAVIC1920x1080I_50HZ 539977 958 15
26: CEAVIC1920x1080P_25HZ 1079954 958 15
27: CEAVIC1920x1080P_50HZ 539977 478 7

so 1080p60 gives a hswidth=7

Tim

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

[Tim Harvey]

On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:

>>
>> I see the AVI infoframe has hdmi_quantization_range and
>> hdmi_ycc_quantization_range along with vid_code.
>>
>> I'm not at all clear what to do with this information. Is there
>> anything you see in the datasheet [1] that points to something I need
>> to be doing?
>
> You can ignore hdmi_ycc_quantization_range, it is the hdmi_quantization_range
> that you need to read out.
>
> The TDA can receive the following formats:
>
> RGB Full Range
> RGB Limited Range
> YUV Bt.601 (aka SMPTE 170M)
> YUV Rec.709
>
> The YUV formats are always limited range.
>
> The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range and
> YUV to be limited range. YUV can be either 601 or 709.
>
> So if the TDA transmits RGB then you need to support the following 
> conversions:
>
> RGB Full -> RGB Full
> RGB Limited -> RGB Full
> YUV 601 -> RGB Full
> YUV 709 -> RGB Full
>
> And if the TDA transmits YUV then you need these conversions:
>
> RGB Full -> YUV601 or YUV709
> RGB Limited -> YUV601 or YUV709
> YUV601 -> YUV601
> YUV709 -> YUV709
>
> For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> 709.
> I recommend to either always convert to YUV601 or to let it depend on the 
> resolution
> (SDTV YUV601, HDTV YUV709).
>

Ok - this is a good explanation that I should be able to follow. I
will make sure to take into account hdmi_quantization_range when I
setup the colorspace conversion matrix for v3.

> Ideally the application should specify what it wants, but we don't have any 
> API
> support for that.
>

>>
>> 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?
>
> The blanking consists of a front porch width, a sync width and a back porch 
> width.
> 'Width' is normally measured in pixels. Vertical blanking is the same, except 
> that
> is measured in lines. All these values are defined in the standards that 
> define these
> timings (e.g. VESA DMT, CTA-861).
>
> So for 1080p60 the active video is 1920x1080. After each line of 1920 pixels 
> you
> have 88 'pixels' of front porch, a sync pulse of 44 'pixels' and a back porch 
> of
> 148 pixels. Total frame width is 2200. Similar for the vertical.
>
> A good HDMI receiver will give you the exact values for these blanking sizes.
> Especially the sync width/height is important since that can provide 
> additional
> format information when receiving VESA formats.
>
> It looks as if the TDA does not measure in exact pixels but in 27 MHz clock
> periods. Which is an approximation only.
>
> So it appears that what you do is the best you can do.
>
> Although I wonder about the hsper: the datasheet suggests that this is the 
> width,
> not a period. What is the value you read out when you receive 1080p60? If that
> would be an exact width, then that would help a lot since you can compare that
> against bt->hsync.

Here's a list of source modes and the vertical/horizontal period and
horizontal pulse width returned from the TDA:
00: VESA640x480P_60HZ 450427 856 101
01: VESA800x600P_60HZ 447620 711 85
02: VESA1024x768P_60HZ 449952 557 55
03: VESA1280x768P_60HZ 450021 568 11
04: VESA1360x768P_60HZ 449867 564 34
05: VESA1280x960P_60HZ 449981 448 55
06: VESA1280x1024P_60HZ 449833 420 27
07: VESA1400x1050P_60HZ 450372 416 7
08: VESA1600x1200P_60HZ 449981 358 27
09: VESA1920x1200P_60HZ 450355 363 4
10: CEAVIC1440x480I_60HZ 450430 1714 123
11: CEAVIC720x480P_60HZ 450430 856 61
12: CEAVIC1280x720P_60HZ 449981 598 13
13: CEAVIC1280x720P_59.94 450431 599 13
14: CEAVIC1920x1080I_60HZ 449981 798 15
15: CEAVIC1920x1080I_59.95HZ 450431 799 15
16: CEAVIC1920x1080P_30HZ 899962 798 15
17: CEAVIC1920x1080P_29.95HZ 900862 799 15
18: CEAVIC1920x1080P_24HZ 1124953 998 15
19: CEAVIC1920x1080P_23.976HZ 1126077 999 15
20: CEAVIC1920x1080P_60HZ 449981 398 7
21: CEAVIC1920x1080P_59.94HZ 450431 399 7
22: CEAVIC1440x576I_50HZ 539976 1726 127
23: CEAVIC720x576P_50HZ 539976 862 63
24: CEAVIC1280x720P_50HZ 539977 718 13
25: CEAVIC1920x1080I_50HZ 539977 958 15
26: CEAVIC1920x1080P_25HZ 1079954 958 15
27: CEAVIC1920x1080P_50HZ 539977 478 7

so 1080p60 gives a hswidth=7

Tim

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

[Hans Verkuil]

On 20/10/17 16:00, Tim Harvey wrote:
> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
> 
 What I am missing here is handling of the RGB quantization range.
 An HDMI receiver will typically send full range RGB or limited range YUV
 to the SoC. The HDMI source can however send full or limited range RGB
 or limited range YUV (full range YUV is theoretically possible, but nobody
 does that).

>>>
>>> isn't this quantization range a function of the colorspace and
>>> colorimetry dictated by the AVI infoframe? I'm taking these into
>>> consideration when setting up the conversion matrix in
>>> tda1997x_configure_conv().
>>
>> No, it's independent of that.
> 
> and from another reply:
>> A small correction here: while ideally you should indeed check if the current
>> EDID supports selectable RGB Quantization Range, in practice you don't need
>> to. If the source explicitly sets the RGB quantization range, then just use
>> that.
>>
>> Note: some hardware can do this automatically (adv7604) by detecting what is
>> transmitted in the AVI InfoFrame. That's probably not the case here since you
>> have to provide a conversion matrix.
> 
> I see the AVI infoframe has hdmi_quantization_range and
> hdmi_ycc_quantization_range along with vid_code.
> 
> I'm not at all clear what to do with this information. Is there
> anything you see in the datasheet [1] that points to something I need
> to be doing?

You can ignore hdmi_ycc_quantization_range, it is the hdmi_quantization_range
that you need to read out.

The TDA can receive the following formats:

RGB Full Range
RGB Limited Range
YUV Bt.601 (aka SMPTE 170M)
YUV Rec.709

The YUV formats are always limited range.

The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range and
YUV to be limited range. YUV can be either 601 or 709.

So if the TDA transmits RGB then you need to support the following conversions:

RGB Full -> RGB Full
RGB Limited -> RGB Full
YUV 601 -> RGB Full
YUV 709 -> RGB Full

And if the TDA transmits YUV then you need these conversions:

RGB Full -> YUV601 or YUV709
RGB Limited -> YUV601 or YUV709
YUV601 -> YUV601
YUV709 -> YUV709

For the RGB to YUV conversion you have a choice of converting to YUV601 or 709.
I recommend to either always convert to YUV601 or to let it depend on the 
resolution
(SDTV YUV601, HDTV YUV709).

Ideally the application should specify what it wants, but we don't have any API
support for that.

> 
>>
>>>
 For a Full HD receiver the rules when receiving RGB video are as follows:

 If the EDID supports selectable RGB Quantization Range, then check if the
 source explicitly sets the RGB quantization range in the AVI InfoFrame and
 use that value.

 Otherwise fall back to the default rules:

 if VIC == 0, then expect full range RGB, otherwise expect limited range 
 RGB.
>>>
>>> Are you referring to the video_code field of the AVI infoframe or vic
>>> from a vendor infoframe?
>>
>> AVI InfoFrame.
>>
>> The HDMI VIC codes in the vendor InfoFrame are only valid for 4k formats. And
>> that's not supported by this device, right?
> 
> Right, the TDA1997x supports 1080p only.
> 
>>
>>>

 It gets even more complicated with 4k video, but this is full HD only.

 In addition, you may also want to implement the V4L2_CID_DV_RX_RGB_RANGE 
 control
 to let userspace override the autodetection.
>>>
>>> I'll add that as an additional patch. Are there other V4L2_CID's that
>>> I should be adding here?
>>
>> V4L2_CID_DV_RX_POWER_PRESENT (if possible) and optionally 
>> V4L2_CID_DV_RX_IT_CONTENT_TYPE.
>>
> 
> It looks like there is a register for 5V_HPD detect.
> 
> I assume the content type to return is the value reported from the AVI frame?

Correct.

> 
> 
>>>
>>> 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 

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

[Hans Verkuil]

On 20/10/17 16:00, Tim Harvey wrote:
> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
> 
 What I am missing here is handling of the RGB quantization range.
 An HDMI receiver will typically send full range RGB or limited range YUV
 to the SoC. The HDMI source can however send full or limited range RGB
 or limited range YUV (full range YUV is theoretically possible, but nobody
 does that).

>>>
>>> isn't this quantization range a function of the colorspace and
>>> colorimetry dictated by the AVI infoframe? I'm taking these into
>>> consideration when setting up the conversion matrix in
>>> tda1997x_configure_conv().
>>
>> No, it's independent of that.
> 
> and from another reply:
>> A small correction here: while ideally you should indeed check if the current
>> EDID supports selectable RGB Quantization Range, in practice you don't need
>> to. If the source explicitly sets the RGB quantization range, then just use
>> that.
>>
>> Note: some hardware can do this automatically (adv7604) by detecting what is
>> transmitted in the AVI InfoFrame. That's probably not the case here since you
>> have to provide a conversion matrix.
> 
> I see the AVI infoframe has hdmi_quantization_range and
> hdmi_ycc_quantization_range along with vid_code.
> 
> I'm not at all clear what to do with this information. Is there
> anything you see in the datasheet [1] that points to something I need
> to be doing?

You can ignore hdmi_ycc_quantization_range, it is the hdmi_quantization_range
that you need to read out.

The TDA can receive the following formats:

RGB Full Range
RGB Limited Range
YUV Bt.601 (aka SMPTE 170M)
YUV Rec.709

The YUV formats are always limited range.

The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range and
YUV to be limited range. YUV can be either 601 or 709.

So if the TDA transmits RGB then you need to support the following conversions:

RGB Full -> RGB Full
RGB Limited -> RGB Full
YUV 601 -> RGB Full
YUV 709 -> RGB Full

And if the TDA transmits YUV then you need these conversions:

RGB Full -> YUV601 or YUV709
RGB Limited -> YUV601 or YUV709
YUV601 -> YUV601
YUV709 -> YUV709

For the RGB to YUV conversion you have a choice of converting to YUV601 or 709.
I recommend to either always convert to YUV601 or to let it depend on the 
resolution
(SDTV YUV601, HDTV YUV709).

Ideally the application should specify what it wants, but we don't have any API
support for that.

> 
>>
>>>
 For a Full HD receiver the rules when receiving RGB video are as follows:

 If the EDID supports selectable RGB Quantization Range, then check if the
 source explicitly sets the RGB quantization range in the AVI InfoFrame and
 use that value.

 Otherwise fall back to the default rules:

 if VIC == 0, then expect full range RGB, otherwise expect limited range 
 RGB.
>>>
>>> Are you referring to the video_code field of the AVI infoframe or vic
>>> from a vendor infoframe?
>>
>> AVI InfoFrame.
>>
>> The HDMI VIC codes in the vendor InfoFrame are only valid for 4k formats. And
>> that's not supported by this device, right?
> 
> Right, the TDA1997x supports 1080p only.
> 
>>
>>>

 It gets even more complicated with 4k video, but this is full HD only.

 In addition, you may also want to implement the V4L2_CID_DV_RX_RGB_RANGE 
 control
 to let userspace override the autodetection.
>>>
>>> I'll add that as an additional patch. Are there other V4L2_CID's that
>>> I should be adding here?
>>
>> V4L2_CID_DV_RX_POWER_PRESENT (if possible) and optionally 
>> V4L2_CID_DV_RX_IT_CONTENT_TYPE.
>>
> 
> It looks like there is a register for 5V_HPD detect.
> 
> I assume the content type to return is the value reported from the AVI frame?

Correct.

> 
> 
>>>
>>> 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 

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

[Tim Harvey]

On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:

>>> What I am missing here is handling of the RGB quantization range.
>>> An HDMI receiver will typically send full range RGB or limited range YUV
>>> to the SoC. The HDMI source can however send full or limited range RGB
>>> or limited range YUV (full range YUV is theoretically possible, but nobody
>>> does that).
>>>
>>
>> isn't this quantization range a function of the colorspace and
>> colorimetry dictated by the AVI infoframe? I'm taking these into
>> consideration when setting up the conversion matrix in
>> tda1997x_configure_conv().
>
> No, it's independent of that.

and from another reply:
> A small correction here: while ideally you should indeed check if the current
> EDID supports selectable RGB Quantization Range, in practice you don't need
> to. If the source explicitly sets the RGB quantization range, then just use
> that.
>
> Note: some hardware can do this automatically (adv7604) by detecting what is
> transmitted in the AVI InfoFrame. That's probably not the case here since you
> have to provide a conversion matrix.

I see the AVI infoframe has hdmi_quantization_range and
hdmi_ycc_quantization_range along with vid_code.

I'm not at all clear what to do with this information. Is there
anything you see in the datasheet [1] that points to something I need
to be doing?

>
>>
>>> For a Full HD receiver the rules when receiving RGB video are as follows:
>>>
>>> If the EDID supports selectable RGB Quantization Range, then check if the
>>> source explicitly sets the RGB quantization range in the AVI InfoFrame and
>>> use that value.
>>>
>>> Otherwise fall back to the default rules:
>>>
>>> if VIC == 0, then expect full range RGB, otherwise expect limited range RGB.
>>
>> Are you referring to the video_code field of the AVI infoframe or vic
>> from a vendor infoframe?
>
> AVI InfoFrame.
>
> The HDMI VIC codes in the vendor InfoFrame are only valid for 4k formats. And
> that's not supported by this device, right?

Right, the TDA1997x supports 1080p only.

>
>>
>>>
>>> It gets even more complicated with 4k video, but this is full HD only.
>>>
>>> In addition, you may also want to implement the V4L2_CID_DV_RX_RGB_RANGE 
>>> control
>>> to let userspace override the autodetection.
>>
>> I'll add that as an additional patch. Are there other V4L2_CID's that
>> I should be adding here?
>
> V4L2_CID_DV_RX_POWER_PRESENT (if possible) and optionally 
> V4L2_CID_DV_RX_IT_CONTENT_TYPE.
>

It looks like there is a register for 5V_HPD detect.

I assume the content type to return is the value reported from the AVI frame?


>>
>> 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 = _hdmi_modes[i];
const struct v4l2_bt_timings *bt = >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 

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

[Tim Harvey]

On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:

>>> What I am missing here is handling of the RGB quantization range.
>>> An HDMI receiver will typically send full range RGB or limited range YUV
>>> to the SoC. The HDMI source can however send full or limited range RGB
>>> or limited range YUV (full range YUV is theoretically possible, but nobody
>>> does that).
>>>
>>
>> isn't this quantization range a function of the colorspace and
>> colorimetry dictated by the AVI infoframe? I'm taking these into
>> consideration when setting up the conversion matrix in
>> tda1997x_configure_conv().
>
> No, it's independent of that.

and from another reply:
> A small correction here: while ideally you should indeed check if the current
> EDID supports selectable RGB Quantization Range, in practice you don't need
> to. If the source explicitly sets the RGB quantization range, then just use
> that.
>
> Note: some hardware can do this automatically (adv7604) by detecting what is
> transmitted in the AVI InfoFrame. That's probably not the case here since you
> have to provide a conversion matrix.

I see the AVI infoframe has hdmi_quantization_range and
hdmi_ycc_quantization_range along with vid_code.

I'm not at all clear what to do with this information. Is there
anything you see in the datasheet [1] that points to something I need
to be doing?

>
>>
>>> For a Full HD receiver the rules when receiving RGB video are as follows:
>>>
>>> If the EDID supports selectable RGB Quantization Range, then check if the
>>> source explicitly sets the RGB quantization range in the AVI InfoFrame and
>>> use that value.
>>>
>>> Otherwise fall back to the default rules:
>>>
>>> if VIC == 0, then expect full range RGB, otherwise expect limited range RGB.
>>
>> Are you referring to the video_code field of the AVI infoframe or vic
>> from a vendor infoframe?
>
> AVI InfoFrame.
>
> The HDMI VIC codes in the vendor InfoFrame are only valid for 4k formats. And
> that's not supported by this device, right?

Right, the TDA1997x supports 1080p only.

>
>>
>>>
>>> It gets even more complicated with 4k video, but this is full HD only.
>>>
>>> In addition, you may also want to implement the V4L2_CID_DV_RX_RGB_RANGE 
>>> control
>>> to let userspace override the autodetection.
>>
>> I'll add that as an additional patch. Are there other V4L2_CID's that
>> I should be adding here?
>
> V4L2_CID_DV_RX_POWER_PRESENT (if possible) and optionally 
> V4L2_CID_DV_RX_IT_CONTENT_TYPE.
>

It looks like there is a register for 5V_HPD detect.

I assume the content type to return is the value reported from the AVI frame?


>>
>> 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 = _hdmi_modes[i];
const struct v4l2_bt_timings *bt = >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)
   

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

[Hans Verkuil]

On 10/19/2017 09:20 AM, Tim Harvey wrote:
> On Wed, Oct 18, 2017 at 5:04 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Here is my review of this v2:
>>
>> On 10/12/17 06:45, Tim Harvey wrote:
>>> Add support for the TDA1997x HDMI receivers.
>>>
> 
>>> +
>>> +/*
>>> + * Video Input formats
>>> + */
>>> +struct vhref_values {
>>> + u16 href_start;
>>> + u16 href_end;
>>> + u16 vref_f1_start;
>>> + u8  vref_f1_width;
>>> + u16 vref_f2_start;
>>> + u8  vref_f2_width;
>>> + u16 fieldref_f1_start;
>>> + u8  fieldPolarity;
>>> + u16 fieldref_f2_start;
>>
>> Since we don't support interlaced (yet) I'd just drop the 'f2' fields.
>> Ditto for fieldPolarity.
>>
>> Can't these href/vref values be calculated from the timings?
>>
> 
> The values in this struct are used to configure the tda1997x VHREF
> timing generator in tda1997x_configure_input_resolution() for
> generating the video output bus timings so I can't really drop them
> unless I can calculate them. Let me look into this - should be
> possible.
> 
>>> +};
>>> +
> 
>>> +/* parse an infoframe and do some sanity checks on it */
>>> +static unsigned int
>>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>>> +{
>>> + struct v4l2_subdev *sd = >sd;
>>> + union hdmi_infoframe frame;
>>> + u8 buffer[40];
>>> + u8 reg;
>>> + int len, err;
>>> +
>>> + /* read data */
>>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>>> + err = hdmi_infoframe_unpack(, buffer);
>>> + if (err) {
>>> + v4l_err(state->client,
>>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>>> + len, addr, buffer[0]);
>>> + return err;
>>> + }
>>> + if (debug > 1)
>>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>>> + switch (frame.any.type) {
>>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>>> + /* sample rate */
>>> + switch (frame.audio.sample_frequency) {
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>>> + state->audio_samplerate = 32000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>>> + state->audio_samplerate = 44100;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>>> + state->audio_samplerate = 48000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>>> + state->audio_samplerate = 88200;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>>> + state->audio_samplerate = 96000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>>> + state->audio_samplerate = 176400;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>>> + state->audio_samplerate = 192000;
>>> + break;
>>> + default:
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>>> + break;
>>> + }
>>> +
>>> + /* sample size */
>>> + switch (frame.audio.sample_size) {
>>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>>> + state->audio_samplesize = 16;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>>> + state->audio_samplesize = 20;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>>> + state->audio_samplesize = 24;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + /* Channel Count */
>>> + state->audio_channels = frame.audio.channels;
>>> + if (frame.audio.channel_allocation &&
>>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>>> + /* use the channel assignment from the infoframe */
>>> + state->audio_ch_alloc = 
>>> frame.audio.channel_allocation;
>>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>>> + /* reset the audio FIFO */
>>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>>> + }
>>> + break;
>>> +
>>> + /* Source Product Descriptor information (SPD) */
>>> + case HDMI_INFOFRAME_TYPE_SPD:
>>> + strncpy(frame.spd.vendor, state->vendor,
>>> + sizeof(frame.spd.vendor));
>>> + strncpy(frame.spd.product, state->product,
>>> + sizeof(frame.spd.product));
>>> + v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>>> 

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

[Hans Verkuil]

On 10/19/2017 09:20 AM, Tim Harvey wrote:
> On Wed, Oct 18, 2017 at 5:04 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Here is my review of this v2:
>>
>> On 10/12/17 06:45, Tim Harvey wrote:
>>> Add support for the TDA1997x HDMI receivers.
>>>
> 
>>> +
>>> +/*
>>> + * Video Input formats
>>> + */
>>> +struct vhref_values {
>>> + u16 href_start;
>>> + u16 href_end;
>>> + u16 vref_f1_start;
>>> + u8  vref_f1_width;
>>> + u16 vref_f2_start;
>>> + u8  vref_f2_width;
>>> + u16 fieldref_f1_start;
>>> + u8  fieldPolarity;
>>> + u16 fieldref_f2_start;
>>
>> Since we don't support interlaced (yet) I'd just drop the 'f2' fields.
>> Ditto for fieldPolarity.
>>
>> Can't these href/vref values be calculated from the timings?
>>
> 
> The values in this struct are used to configure the tda1997x VHREF
> timing generator in tda1997x_configure_input_resolution() for
> generating the video output bus timings so I can't really drop them
> unless I can calculate them. Let me look into this - should be
> possible.
> 
>>> +};
>>> +
> 
>>> +/* parse an infoframe and do some sanity checks on it */
>>> +static unsigned int
>>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>>> +{
>>> + struct v4l2_subdev *sd = >sd;
>>> + union hdmi_infoframe frame;
>>> + u8 buffer[40];
>>> + u8 reg;
>>> + int len, err;
>>> +
>>> + /* read data */
>>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>>> + err = hdmi_infoframe_unpack(, buffer);
>>> + if (err) {
>>> + v4l_err(state->client,
>>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>>> + len, addr, buffer[0]);
>>> + return err;
>>> + }
>>> + if (debug > 1)
>>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>>> + switch (frame.any.type) {
>>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>>> + /* sample rate */
>>> + switch (frame.audio.sample_frequency) {
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>>> + state->audio_samplerate = 32000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>>> + state->audio_samplerate = 44100;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>>> + state->audio_samplerate = 48000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>>> + state->audio_samplerate = 88200;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>>> + state->audio_samplerate = 96000;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>>> + state->audio_samplerate = 176400;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>>> + state->audio_samplerate = 192000;
>>> + break;
>>> + default:
>>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>>> + break;
>>> + }
>>> +
>>> + /* sample size */
>>> + switch (frame.audio.sample_size) {
>>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>>> + state->audio_samplesize = 16;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>>> + state->audio_samplesize = 20;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>>> + state->audio_samplesize = 24;
>>> + break;
>>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + /* Channel Count */
>>> + state->audio_channels = frame.audio.channels;
>>> + if (frame.audio.channel_allocation &&
>>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>>> + /* use the channel assignment from the infoframe */
>>> + state->audio_ch_alloc = 
>>> frame.audio.channel_allocation;
>>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>>> + /* reset the audio FIFO */
>>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>>> + }
>>> + break;
>>> +
>>> + /* Source Product Descriptor information (SPD) */
>>> + case HDMI_INFOFRAME_TYPE_SPD:
>>> + strncpy(frame.spd.vendor, state->vendor,
>>> + sizeof(frame.spd.vendor));
>>> + strncpy(frame.spd.product, state->product,
>>> + sizeof(frame.spd.product));
>>> + v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>>> +

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

[Tim Harvey]

On Wed, Oct 18, 2017 at 5:04 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Here is my review of this v2:
>
> On 10/12/17 06:45, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>

>> +
>> +/*
>> + * Video Input formats
>> + */
>> +struct vhref_values {
>> + u16 href_start;
>> + u16 href_end;
>> + u16 vref_f1_start;
>> + u8  vref_f1_width;
>> + u16 vref_f2_start;
>> + u8  vref_f2_width;
>> + u16 fieldref_f1_start;
>> + u8  fieldPolarity;
>> + u16 fieldref_f2_start;
>
> Since we don't support interlaced (yet) I'd just drop the 'f2' fields.
> Ditto for fieldPolarity.
>
> Can't these href/vref values be calculated from the timings?
>

The values in this struct are used to configure the tda1997x VHREF
timing generator in tda1997x_configure_input_resolution() for
generating the video output bus timings so I can't really drop them
unless I can calculate them. Let me look into this - should be
possible.

>> +};
>> +

>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = >sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + if (debug > 1)
>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> + /* reset the audio FIFO */
>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> + }
>> + break;
>> +
>> + /* Source Product Descriptor information (SPD) */
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> + strncpy(frame.spd.vendor, state->vendor,
>> + sizeof(frame.spd.vendor));
>> + strncpy(frame.spd.product, state->product,
>> + sizeof(frame.spd.product));
>> + v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>> +  state->vendor, state->product);
>
> Use hdmi_infoframe_log() for logging infoframes.

ok - I will always call hdmi_infoframe_log() above and refrain from
outputs 

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

[Tim Harvey]

On Wed, Oct 18, 2017 at 5:04 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Here is my review of this v2:
>
> On 10/12/17 06:45, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>

>> +
>> +/*
>> + * Video Input formats
>> + */
>> +struct vhref_values {
>> + u16 href_start;
>> + u16 href_end;
>> + u16 vref_f1_start;
>> + u8  vref_f1_width;
>> + u16 vref_f2_start;
>> + u8  vref_f2_width;
>> + u16 fieldref_f1_start;
>> + u8  fieldPolarity;
>> + u16 fieldref_f2_start;
>
> Since we don't support interlaced (yet) I'd just drop the 'f2' fields.
> Ditto for fieldPolarity.
>
> Can't these href/vref values be calculated from the timings?
>

The values in this struct are used to configure the tda1997x VHREF
timing generator in tda1997x_configure_input_resolution() for
generating the video output bus timings so I can't really drop them
unless I can calculate them. Let me look into this - should be
possible.

>> +};
>> +

>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = >sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + if (debug > 1)
>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> + /* reset the audio FIFO */
>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> + }
>> + break;
>> +
>> + /* Source Product Descriptor information (SPD) */
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> + strncpy(frame.spd.vendor, state->vendor,
>> + sizeof(frame.spd.vendor));
>> + strncpy(frame.spd.product, state->product,
>> + sizeof(frame.spd.product));
>> + v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>> +  state->vendor, state->product);
>
> Use hdmi_infoframe_log() for logging infoframes.

ok - I will always call hdmi_infoframe_log() above and refrain from
outputs that just repeat 

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

[Hans Verkuil]

On 10/18/2017 02:04 PM, Hans Verkuil wrote:
> Hi Tim,
> 
> Here is my review of this v2:
> 
> On 10/12/17 06:45, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - use new audio bindings
>>
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3336 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>>  include/media/i2c/tda1997x.h |   53 +
>>  5 files changed, 3477 insertions(+)
>>  create mode 100644 drivers/media/i2c/tda1997x.c
>>  create mode 100644 include/dt-bindings/media/tda1997x.h
>>  create mode 100644 include/media/i2c/tda1997x.h
>>



>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> +struct v4l2_subdev *sd = >sd;
>> +union hdmi_infoframe frame;
>> +u8 buffer[40];
>> +u8 reg;
>> +int len, err;
>> +
>> +/* read data */
>> +len = io_readn(sd, addr, sizeof(buffer), buffer);
>> +err = hdmi_infoframe_unpack(, buffer);
>> +if (err) {
>> +v4l_err(state->client,
>> +"failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> +len, addr, buffer[0]);
>> +return err;
>> +}
>> +if (debug > 1)
>> +hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> +switch (frame.any.type) {
>> +/* Audio InfoFrame: see HDMI spec 8.2.2 */
>> +case HDMI_INFOFRAME_TYPE_AUDIO:
>> +/* sample rate */
>> +switch (frame.audio.sample_frequency) {
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> +state->audio_samplerate = 32000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> +state->audio_samplerate = 44100;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> +state->audio_samplerate = 48000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> +state->audio_samplerate = 88200;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> +state->audio_samplerate = 96000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> +state->audio_samplerate = 176400;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> +state->audio_samplerate = 192000;
>> +break;
>> +default:
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> +break;
>> +}
>> +
>> +/* sample size */
>> +switch (frame.audio.sample_size) {
>> +case HDMI_AUDIO_SAMPLE_SIZE_16:
>> +state->audio_samplesize = 16;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_20:
>> +state->audio_samplesize = 20;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_24:
>> +state->audio_samplesize = 24;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> +default:
>> +break;
>> +}
>> +
>> +/* Channel Count */
>> +state->audio_channels = frame.audio.channels;
>> +if (frame.audio.channel_allocation &&
>> +frame.audio.channel_allocation != state->audio_ch_alloc) {
>> +/* use the channel assignment from the infoframe */
>> +state->audio_ch_alloc = frame.audio.channel_allocation;
>> +tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> +/* reset the audio FIFO */
>> +tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> +}
>> +break;
>> +
>> +/* Source Product Descriptor information (SPD) */
>> +case HDMI_INFOFRAME_TYPE_SPD:
>> +strncpy(frame.spd.vendor, state->vendor,
>> +sizeof(frame.spd.vendor));
>> +strncpy(frame.spd.product, state->product,
>> +sizeof(frame.spd.product));
>> +v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>> + state->vendor, state->product);
> 
> Use hdmi_infoframe_log() for logging infoframes.
> 
>> +break;
>> +
>> +/* Auxiliary Video information (AVI) InfoFrame: see HDMI spec 8.2.1 */
>> 

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

[Hans Verkuil]

On 10/18/2017 02:04 PM, Hans Verkuil wrote:
> Hi Tim,
> 
> Here is my review of this v2:
> 
> On 10/12/17 06:45, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - use new audio bindings
>>
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3336 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>>  include/media/i2c/tda1997x.h |   53 +
>>  5 files changed, 3477 insertions(+)
>>  create mode 100644 drivers/media/i2c/tda1997x.c
>>  create mode 100644 include/dt-bindings/media/tda1997x.h
>>  create mode 100644 include/media/i2c/tda1997x.h
>>



>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> +struct v4l2_subdev *sd = >sd;
>> +union hdmi_infoframe frame;
>> +u8 buffer[40];
>> +u8 reg;
>> +int len, err;
>> +
>> +/* read data */
>> +len = io_readn(sd, addr, sizeof(buffer), buffer);
>> +err = hdmi_infoframe_unpack(, buffer);
>> +if (err) {
>> +v4l_err(state->client,
>> +"failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> +len, addr, buffer[0]);
>> +return err;
>> +}
>> +if (debug > 1)
>> +hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> +switch (frame.any.type) {
>> +/* Audio InfoFrame: see HDMI spec 8.2.2 */
>> +case HDMI_INFOFRAME_TYPE_AUDIO:
>> +/* sample rate */
>> +switch (frame.audio.sample_frequency) {
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> +state->audio_samplerate = 32000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> +state->audio_samplerate = 44100;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> +state->audio_samplerate = 48000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> +state->audio_samplerate = 88200;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> +state->audio_samplerate = 96000;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> +state->audio_samplerate = 176400;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> +state->audio_samplerate = 192000;
>> +break;
>> +default:
>> +case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> +break;
>> +}
>> +
>> +/* sample size */
>> +switch (frame.audio.sample_size) {
>> +case HDMI_AUDIO_SAMPLE_SIZE_16:
>> +state->audio_samplesize = 16;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_20:
>> +state->audio_samplesize = 20;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_24:
>> +state->audio_samplesize = 24;
>> +break;
>> +case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> +default:
>> +break;
>> +}
>> +
>> +/* Channel Count */
>> +state->audio_channels = frame.audio.channels;
>> +if (frame.audio.channel_allocation &&
>> +frame.audio.channel_allocation != state->audio_ch_alloc) {
>> +/* use the channel assignment from the infoframe */
>> +state->audio_ch_alloc = frame.audio.channel_allocation;
>> +tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> +/* reset the audio FIFO */
>> +tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> +}
>> +break;
>> +
>> +/* Source Product Descriptor information (SPD) */
>> +case HDMI_INFOFRAME_TYPE_SPD:
>> +strncpy(frame.spd.vendor, state->vendor,
>> +sizeof(frame.spd.vendor));
>> +strncpy(frame.spd.product, state->product,
>> +sizeof(frame.spd.product));
>> +v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>> + state->vendor, state->product);
> 
> Use hdmi_infoframe_log() for logging infoframes.
> 
>> +break;
>> +
>> +/* Auxiliary Video information (AVI) InfoFrame: see HDMI spec 8.2.1 */
>> +case HDMI_INFOFRAME_TYPE_AVI:
>> + 

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

[Hans Verkuil]

Hi Tim,

Here is my review of this v2:

On 10/12/17 06:45, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - use new audio bindings
> 
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3336 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +
>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3477 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9415389..c2b0400 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -56,6 +56,15 @@ config VIDEO_TDA9840
> To compile this driver as a module, choose M here: the
> module will be called tda9840.
>  
> +config VIDEO_TDA1997X
> + tristate "NXP TDA1997x HDMI receiver"
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called tda1997x.
> +
>  config VIDEO_TEA6415C
>   tristate "Philips TEA6415C audio processor"
>   depends on I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c843c18..58f2b2e 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
>  obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>  obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>  obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
> +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>  obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
>  obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
>  obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> new file mode 100644
> index 000..bf06684
> --- /dev/null
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -0,0 +1,3336 @@
> +/*
> + * Copyright (C) 2017 Gateworks Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* debug level */
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* Page 0x00 - General Control */
> +#define REG_VERSION  0x
> +#define REG_INPUT_SEL0x0001
> +#define REG_SVC_MODE 0x0002
> +#define REG_HPD_MAN_CTRL 0x0003
> +#define REG_RT_MAN_CTRL  0x0004
> +#define REG_STANDBY_SOFT_RST 0x000A
> +#define REG_HDMI_SOFT_RST0x000B
> +#define REG_HDMI_INFO_RST0x000C
> +#define REG_INT_FLG_CLR_TOP  0x000E
> +#define REG_INT_FLG_CLR_SUS  0x000F
> +#define REG_INT_FLG_CLR_DDC  0x0010
> +#define REG_INT_FLG_CLR_RATE 0x0011
> +#define REG_INT_FLG_CLR_MODE 0x0012
> +#define REG_INT_FLG_CLR_INFO 0x0013
> +#define REG_INT_FLG_CLR_AUDIO0x0014
> +#define REG_INT_FLG_CLR_HDCP 0x0015
> +#define REG_INT_FLG_CLR_AFE  0x0016
> +#define REG_INT_MASK_TOP 0x0017
> +#define REG_INT_MASK_SUS 0x0018
> +#define REG_INT_MASK_DDC 0x0019
> +#define REG_INT_MASK_RATE0x001A
> +#define REG_INT_MASK_MODE0x001B
> +#define REG_INT_MASK_INFO0x001C
> +#define REG_INT_MASK_AUDIO   0x001D
> +#define REG_INT_MASK_HDCP0x001E
> +#define REG_INT_MASK_AFE 0x001F
> +#define REG_DETECT_5V0x0020
> +#define REG_SUS_STATUS   0x0021
> +#define REG_V_PER0x0022
> +#define REG_H_PER0x0025
> +#define REG_HS_WIDTH 0x0027
> +#define REG_FMT_H_TOT0x0029
> +#define REG_FMT_H_ACT0x002b
> +#define REG_FMT_H_FRONT  0x002d
> +#define REG_FMT_H_SYNC   0x002f
> +#define REG_FMT_H_BACK   0x0031
> +#define REG_FMT_V_TOT0x0033
> +#define REG_FMT_V_ACT0x0035
> +#define REG_FMT_V_FRONT_F1   0x0037
> +#define REG_FMT_V_FRONT_F2   0x0038
> +#define REG_FMT_V_SYNC   0x0039
> +#define 

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

[Hans Verkuil]

Hi Tim,

Here is my review of this v2:

On 10/12/17 06:45, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - use new audio bindings
> 
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3336 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +
>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3477 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9415389..c2b0400 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -56,6 +56,15 @@ config VIDEO_TDA9840
> To compile this driver as a module, choose M here: the
> module will be called tda9840.
>  
> +config VIDEO_TDA1997X
> + tristate "NXP TDA1997x HDMI receiver"
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called tda1997x.
> +
>  config VIDEO_TEA6415C
>   tristate "Philips TEA6415C audio processor"
>   depends on I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c843c18..58f2b2e 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
>  obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>  obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>  obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
> +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>  obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
>  obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
>  obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> new file mode 100644
> index 000..bf06684
> --- /dev/null
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -0,0 +1,3336 @@
> +/*
> + * Copyright (C) 2017 Gateworks Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* debug level */
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* Page 0x00 - General Control */
> +#define REG_VERSION  0x
> +#define REG_INPUT_SEL0x0001
> +#define REG_SVC_MODE 0x0002
> +#define REG_HPD_MAN_CTRL 0x0003
> +#define REG_RT_MAN_CTRL  0x0004
> +#define REG_STANDBY_SOFT_RST 0x000A
> +#define REG_HDMI_SOFT_RST0x000B
> +#define REG_HDMI_INFO_RST0x000C
> +#define REG_INT_FLG_CLR_TOP  0x000E
> +#define REG_INT_FLG_CLR_SUS  0x000F
> +#define REG_INT_FLG_CLR_DDC  0x0010
> +#define REG_INT_FLG_CLR_RATE 0x0011
> +#define REG_INT_FLG_CLR_MODE 0x0012
> +#define REG_INT_FLG_CLR_INFO 0x0013
> +#define REG_INT_FLG_CLR_AUDIO0x0014
> +#define REG_INT_FLG_CLR_HDCP 0x0015
> +#define REG_INT_FLG_CLR_AFE  0x0016
> +#define REG_INT_MASK_TOP 0x0017
> +#define REG_INT_MASK_SUS 0x0018
> +#define REG_INT_MASK_DDC 0x0019
> +#define REG_INT_MASK_RATE0x001A
> +#define REG_INT_MASK_MODE0x001B
> +#define REG_INT_MASK_INFO0x001C
> +#define REG_INT_MASK_AUDIO   0x001D
> +#define REG_INT_MASK_HDCP0x001E
> +#define REG_INT_MASK_AFE 0x001F
> +#define REG_DETECT_5V0x0020
> +#define REG_SUS_STATUS   0x0021
> +#define REG_V_PER0x0022
> +#define REG_H_PER0x0025
> +#define REG_HS_WIDTH 0x0027
> +#define REG_FMT_H_TOT0x0029
> +#define REG_FMT_H_ACT0x002b
> +#define REG_FMT_H_FRONT  0x002d
> +#define REG_FMT_H_SYNC   0x002f
> +#define REG_FMT_H_BACK   0x0031
> +#define REG_FMT_V_TOT0x0033
> +#define REG_FMT_V_ACT0x0035
> +#define REG_FMT_V_FRONT_F1   0x0037
> +#define REG_FMT_V_FRONT_F2   0x0038
> +#define REG_FMT_V_SYNC   0x0039
> +#define REG_FMT_V_BACK_F10x003a
> +#define