Re: [RFC 2/2] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Hans Verkuil]

On 05/23/2017 06:38 PM, Jose Abreu wrote:
> Hi Hans,
> 
> 
> Thanks for the review!
> 
> On 22-05-2017 11:36, Hans Verkuil wrote:
>> On 04/21/2017 11:53 AM, Jose Abreu wrote:

<snip>

>>> +static int dw_hdmi_query_dv_timings(struct v4l2_subdev *sd,
>>> +		struct v4l2_dv_timings *timings)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>>> +	struct v4l2_bt_timings *bt = &timings->bt;
>>> +	u32 htot, hofs;
>>> +	u32 vtot;
>>> +	u8 vic;
>>> +
>>> +	dev_dbg(dw_dev->dev, "%s\n", __func__);
>>> +
>>> +	memset(timings, 0, sizeof(*timings));
>>> +
>>> +	timings->type = V4L2_DV_BT_656_1120;
>>> +	bt->width = hdmi_readl(dw_dev, HDMI_MD_HACT_PX);
>>> +	bt->height = hdmi_readl(dw_dev, HDMI_MD_VAL);
>>> +	bt->interlaced = 0;
>> There is no interlaced support? Most receivers can at least detect it.
> 
> The controller supports interlaced, unfortunately there is no way
> I can test it, so we chose not to add it in the driver.

But isn't there a register that tells you if the source was interlaced?
Almost all HDMI receiver drivers can detect this, even though they don't
actually allow interlaced formats to be used. It is disabled in the
DV timings capabilities.

My problem with this code is that it doesn't tell the caller that the
signal is interlaced. This can lead to problems where it is incorrectly
interpreted as progressive.

> 
>>
>>> +
>>> +	if (hdmi_readl(dw_dev, HDMI_ISTS) & HDMI_ISTS_VS_POL_ADJ)
>>> +		bt->polarities |= V4L2_DV_VSYNC_POS_POL;
>>> +	if (hdmi_readl(dw_dev, HDMI_ISTS) & HDMI_ISTS_HS_POL_ADJ)
>>> +		bt->polarities |= V4L2_DV_HSYNC_POS_POL;
>>> +
>>> +	bt->pixelclock = dw_hdmi_get_pixelclk(dw_dev);
>> Can this be rounded up to a value above 594 MHz? In the timings cap that
>> is the max frequency, but you probably need to allow for a bit of margin there
>> in case you measure e.g. 594050000 Hz.
> 
> Hmm, yeah, probably it can. Actually the timings cap may not be
> correct because we support deep color in 4k, so freq will be >
> 594MHz.

No, since this is the pixel clock, and the number of pixels remains the same,
even when using deep color.

It is increasingly likely that this driver might be the first to support
deep color, so it is very possible that some API changes may have to be made.

I always wanted to add support for this, but I never had the chance.

>>> +static int dw_hdmi_s_register(struct v4l2_subdev *sd,
>>> +		const struct v4l2_dbg_register *reg)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>>> +
>>> +	switch (reg->reg >> 15) {
>>> +	case 0: /* Controller core write */
>>> +		hdmi_writel(dw_dev, reg->val & GENMASK(31,0), reg->reg & 0x7fff);
>>> +		return 0;
>>> +	case 1: /* PHY write */
>>> +		if ((reg->reg & ~0xff) != BIT(15))
>>> +			break;
>>> +		dw_hdmi_phy_write(dw_dev, reg->val & 0xffff, reg->reg & 0xff);
>>> +		return 0;
>>> +	default:
>>> +		break;
>>> +	}
>> Be careful here: if you support HDCP, then you typically don't want to allow
>> userspace to touch any HDCP-related registers through this API.
> 
> Yeah, HDCP is supported but still not implemented. Still, the
> only thing the user will be able to change will be bksv because
> keys can not be read, they are write only. I will add a check though.

Let me know if/when you want to add actual HDCP support. I have some old patches
for HDCP that we (Cisco) made a long time ago.

Regards,

	Hans