Message-ID: <0ebac69e-8de3-e65e-d6f5-dfb4fed3585c@xs4all.nl>
Date:   Fri, 16 Jun 2017 15:44:19 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Jose Abreu <Jose.Abreu@...opsys.com>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Cc:     Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH v2 2/4] [media] platform: Add Synopsys Designware HDMI RX
 Controller Driver

On 06/16/17 15:21, Jose Abreu wrote:
> Hi Hans,
> 
> 
> On 16-06-2017 13:38, Hans Verkuil wrote:
>> Hi Jose,
>>
>> Just a quick review of the new CEC code:
> 
> Thanks!
> 
>>
>> On 06/16/17 12:07, Jose Abreu wrote:
>>> This is an initial submission for the Synopsys Designware HDMI RX
>>> Controller Driver. This driver interacts with a phy driver so that
>>> a communication between them is created and a video pipeline is
>>> configured.
>>>
>>> The controller + phy pipeline can then be integrated into a fully
>>> featured system that can be able to receive video up to 4k@...z
>>> with deep color 48bit RGB, depending on the platform. Although,
>>> this initial version does not yet handle deep color modes.
>>>
>>> This driver was implemented as a standard V4L2 subdevice and its
>>> main features are:
>>> 	- Internal state machine that reconfigures phy until the
>>> 	video is not stable
>>> 	- JTAG communication with phy
>>> 	- Inter-module communication with phy driver
>>> 	- Debug write/read ioctls
>>>
>>> Some notes:
>>> 	- RX sense controller (cable connection/disconnection) must
>>> 	be handled by the platform wrapper as this is not integrated
>>> 	into the controller RTL
>>> 	- The same goes for EDID ROM's
>>> 	- ZCAL calibration is needed only in FPGA platforms, in ASIC
>>> 	this is not needed
>>> 	- The state machine is not an ideal solution as it creates a
>>> 	kthread but it is needed because some sources might not be
>>> 	very stable at sending the video (i.e. we must react
>>> 	accordingly).
>>>
>>> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
>>> Cc: Carlos Palminha <palminha@...opsys.com>
>>> Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
>>> Cc: Hans Verkuil <hans.verkuil@...co.com>
>>>
>>> Changes from v1:
>>> 	- Added support for CEC
>>> 	- Correct typo errors
>>> 	- Correctly detect interlaced video modes
>>> 	- Correct VIC parsing
>>> Changes from RFC:
>>> 	- Added support for HDCP 1.4
>>> 	- Fixup HDMI_VIC not being parsed (Hans)
>>> 	- Send source change signal when powering off (Hans)
>>> 	- Added a "wait stable delay"
>>> 	- Detect interlaced video modes (Hans)
>>> 	- Restrain g/s_register from reading/writing to HDCP regs (Hans)
>>>
>>> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
>>> ---
>>>  drivers/media/platform/dwc/Kconfig      |   10 +
>>>  drivers/media/platform/dwc/Makefile     |    1 +
>>>  drivers/media/platform/dwc/dw-hdmi-rx.c | 1764 +++++++++++++++++++++++++++++++
>>>  drivers/media/platform/dwc/dw-hdmi-rx.h |  435 ++++++++
>>>  include/media/dwc/dw-hdmi-rx-pdata.h    |   63 ++
>>>  5 files changed, 2273 insertions(+)
>>>  create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c
>>>  create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h
>>>  create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h
>>> diff --git a/drivers/media/platform/dwc/Kconfig b/drivers/media/platform/dwc/Kconfig
>>> index 361d38d..aba18af 100644
>>> --- a/drivers/media/platform/dwc/Kconfig
>>> +++ b/drivers/media/platform/dwc/Kconfig
>>> @@ -6,3 +6,13 @@ config VIDEO_DWC_HDMI_PHY_E405
>>>  
>>>  	  To compile this driver as a module, choose M here. The module
>>>  	  will be called dw-hdmi-phy-e405.
>>> +
>>> +config VIDEO_DWC_HDMI_RX
>>> +	tristate "Synopsys Designware HDMI Receiver driver"
>>> +	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>>> +	select CEC_CORE
>>> +	help
>>> +	  Support for Synopsys Designware HDMI RX controller.
>>> +
>>> +	  To compile this driver as a module, choose M here. The module
>>> +	  will be called dw-hdmi-rx.
>> Add a separate config VIDEO_DWC_HDMI_RX_CEC to explicitly enable
>> CEC. CEC is an optional component of HDMI and supporting CEC should
>> be something you select in the kernel config.
> 
> Ok, will add, but see bellow.
> 
>>
>> <snip>
>>
>> General question: is this CEC IP the same IP as is used in the HDMI
>> transmitter? If so, then it is not a good idea to duplicate the
>> code.
>>
>> If they are different, then that should be noted in the source.
> 
> Well, at regbank they are :) I mean, CEC is not a separate IP,
> its directly integrated in the controller so they probably just
> reused the block in TX and in RX.

I compared the registers of this driver with that of Russell's CEC
driver, and esp. the interrupt handling part is quite different.

Since that's really the meat of the CEC code, I agree that it is
OK to keep this separate.

Perhaps a comment about these differences might be useful.

> CEC is optional in the
> controller configuration (as well as many other functionalities).
> Unfortunately there is no register in this controller that can be
> used to tell the configuration that was used (i.e. tell if CEC
> interface is present). I could read registers and check against
> default values, but that doesn't seem very nice. Either way I
> think the CEC Kconfig bool in this should be hidden from users
> and just let the wrapper driver activate if needed, because the
> wrapper driver will probably know if the platform has cec support
> or not. Or we could add a node in the DT.

As long as the CEC part can be disabled if it is not needed. It
avoid having to load the cec.ko module.

> 
>>
>>> +static void dw_hdmi_cec_enable_ints(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +	u32 mask = HDMI_AUD_CEC_ISTS_DONE | HDMI_AUD_CEC_ISTS_EOM |
>>> +		HDMI_AUD_CEC_ISTS_NACK | HDMI_AUD_CEC_ISTS_ARBLST |
>>> +		HDMI_AUD_CEC_ISTS_ERROR_INIT | HDMI_AUD_CEC_ISTS_ERROR_FOLL;
>>> +
>>> +	hdmi_writel(dw_dev, mask, HDMI_AUD_CEC_IEN_SET);
>>> +	hdmi_writel(dw_dev, 0x0, HDMI_CEC_MASK);
>>> +}
>>> +
>>> +static void dw_hdmi_cec_disable_ints(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +	hdmi_writel(dw_dev, ~0x0, HDMI_AUD_CEC_IEN_CLR);
>>> +	hdmi_writel(dw_dev, ~0x0, HDMI_CEC_MASK);
>>> +}
>>> +
>>> +static void dw_hdmi_cec_clear_ints(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +	hdmi_writel(dw_dev, ~0x0, HDMI_AUD_CEC_ICLR);
>>> +}
>>> +
>>> +static void dw_hdmi_cec_tx_raw_status(struct dw_hdmi_dev *dw_dev, u32 stat)
>>> +{
>>> +	u32 error_mask = HDMI_AUD_CEC_ISTS_NACK |
>>> +		HDMI_AUD_CEC_ISTS_ERROR_INIT;
>>> +
>>> +	if (hdmi_readl(dw_dev, HDMI_CEC_CTRL) & HDMI_CEC_CTRL_SEND_MASK) {
>>> +		dev_dbg(dw_dev->dev, "%s: tx is busy\n", __func__);
>>> +		return;
>>> +	}
>>> +
>>> +	if (stat & HDMI_AUD_CEC_ISTS_ARBLST) {
>>> +		dev_dbg(dw_dev->dev, "%s: arbitration lost\n", __func__);
>>> +		cec_transmit_done(dw_dev->cec_adap, CEC_TX_STATUS_ARB_LOST,
>>> +				1, 0, 0, 0);
>>> +		return;
>>> +	}
>>> +
>>> +	if (stat & HDMI_AUD_CEC_ISTS_DONE) {
>>> +		dev_dbg(dw_dev->dev, "%s: transmission done\n", __func__);
>>> +		cec_transmit_done(dw_dev->cec_adap, CEC_TX_STATUS_OK, 0, 0, 0, 0);
>>> +		return;
>>> +	}
>>> +
>>> +	if (stat & error_mask) {
>>> +		/* If we reached here we have an error */
>>> +		u8 status = 0, nack_cnt = 0, error_cnt = 0;
>>> +
>>> +		dev_dbg(dw_dev->dev, "%s: error found (stat=0x%x)\n", __func__,
>>> +				stat);
>>> +
>>> +		if (stat & HDMI_AUD_CEC_ISTS_NACK) {
>>> +			status |= CEC_TX_STATUS_NACK;
>>> +			nack_cnt++;
>>> +		}
>>> +
>>> +		if (stat & HDMI_AUD_CEC_ISTS_ERROR_INIT) {
>>> +			status |= CEC_TX_STATUS_ERROR;
>>> +			error_cnt++;
>>> +		}
>> What does it mean if you have both NACK and ERROR_INIT flags set? Is that
>> an error or a NACK? You have to chose here which one you pick when calling
>> cec_transmit_done.
> 
> I don't know if these can happen simultaneously. I remember I
> could provoke NACK's but I never got ERROR_INIT.

I would put an else before the ERROR_INIT if. The combination makes no sense
and it would confuse the CEC framework.

> 
>>
>>> +
>>> +		cec_transmit_done(dw_dev->cec_adap, status, 0, nack_cnt, 0,
>>> +				error_cnt);
>>> +		return;
>>> +	}
>>> +}
>>> +
>>> +static void dw_hdmi_cec_received_msg(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +	struct cec_msg msg;
>>> +	u8 i;
>>> +
>>> +	msg.len = hdmi_readl(dw_dev, HDMI_CEC_RX_CNT);
>>> +	if (!msg.len || msg.len > HDMI_CEC_RX_DATA_MAX)
>>> +		return; /* its an invalid/non-existent message */
>> it's
>>
>>> +
>>> +	for (i = 0; i < msg.len; i++)
>>> +		msg.msg[i] = hdmi_readl(dw_dev, HDMI_CEC_RX_DATA(i));
>>> +
>>> +	hdmi_writel(dw_dev, 0x0, HDMI_CEC_LOCK);
>>> +	cec_received_msg(dw_dev->cec_adap, &msg);
>>> +}
>>> +
>>> +static int dw_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = cec_get_drvdata(adap);
>>> +
>>> +	if (!dw_dev->cec_enabled_adap && enable) {
>>> +		hdmi_writel(dw_dev, 0x0, HDMI_CEC_LOCK);
>>> +		dw_hdmi_cec_enable_ints(dw_dev);
>>> +	} else if (dw_dev->cec_enabled_adap && !enable) {
>>> +		dw_hdmi_cec_disable_ints(dw_dev);
>>> +		dw_dev->cec_valid_addrs = 0;
>>> +	}
>>> +
>>> +	dw_dev->cec_enabled_adap = enable;
>>> +	return 0;
>>> +}
>>> +
>>> +static int dw_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = cec_get_drvdata(adap);
>>> +	int i, free_idx = HDMI_CEC_MAX_LOG_ADDRS;
>>> +	u32 tmp;
>>> +
>>> +	if (!dw_dev->cec_enabled_adap)
>>> +		return addr == CEC_LOG_ADDR_INVALID ? 0 : -EIO;
>> This can't be called unless the adapter is enabled. So this check
>> can be removed.
>>
>>> +
>>> +	if (addr == CEC_LOG_ADDR_INVALID) {
>>> +		hdmi_writel(dw_dev, 0x0, HDMI_CEC_ADDR_L);
>>> +		hdmi_writel(dw_dev, 0x0, HDMI_CEC_ADDR_H);
>>> +		dw_dev->cec_valid_addrs = 0;
>>> +		return 0;
>>> +	}
>>> +
>>> +	for (i = 0; i < HDMI_CEC_MAX_LOG_ADDRS; i++) {
>>> +		bool is_valid = dw_dev->cec_valid_addrs & (1 << i);
>>> +
>>> +		if (free_idx == HDMI_CEC_MAX_LOG_ADDRS && !is_valid)
>>> +			free_idx = i;
>>> +		if (is_valid && dw_dev->cec_addr[i] == addr)
>>> +			return 0;
>>> +	}
>>> +
>>> +	if (i == HDMI_CEC_MAX_LOG_ADDRS) {
>>> +		i = free_idx;
>>> +		if (i == HDMI_CEC_MAX_LOG_ADDRS)
>>> +			return -ENXIO;
>>> +	}
>>> +
>>> +	dw_dev->cec_addr[i] = addr;
>>> +	dw_dev->cec_valid_addrs |= 1 << i;
>>> +
>>> +	if (i >= 8) {
>>> +		tmp = hdmi_readl(dw_dev, HDMI_CEC_ADDR_H);
>>> +		tmp |= BIT(i - 8);
>>> +		hdmi_writel(dw_dev, tmp, HDMI_CEC_ADDR_H);
>>> +	} else {
>>> +		tmp = hdmi_readl(dw_dev, HDMI_CEC_ADDR_L);
>>> +		tmp |= BIT(i);
>>> +		hdmi_writel(dw_dev, tmp, HDMI_CEC_ADDR_L);
>>> +	}
>> This seems overengineered. HDMI_CEC_ADDR_H/L is just a bitmask, so just set the
>> bit corresponding to addr. I'm not sure what the point of cec_addr[] and cec_valid_addrs
>> is.
> 
> You mean I can only select between these:
> 
> ADDR_L:
> Logical address 0 - Device TV
> Logical address 1 - Recording Device 1
> Logical address 2 - Recording Device 2
> Logical address 3 - Tuner 1
> Logical address 4 - Playback Device 1
> Logical address 5 - Audio System
> Logical address 6 - Tuner 2
> Logical address 7 - Tuner 3
> 
> ADDR_H:
> Logical address 8 - Playback Device 2
> Logical address 9 - Playback Device 3
> Logical address 10 - Tuner 4
> Logical address 11 - Playback Device 3
> Logical address 12 - Reserved
> Logical address 13 - Reserved
> Logical address 14 - Free use
> Logical address 15 - Unregistered (as initiator address), Broadcast
> (as destination address)
> 
> ?
> 
> But I still need to know if I got exhausted of logical addresses,
> right? (And now I realized this code is not respecting the
> logical addresses allocation :/)

You tell the framework about the maximum number of LAs that you
support simultaneously when you call cec_allocate_adapter. The framework
will take care of checking that that max is never exceeded. No need
to check again.

You're asked here to either clear the logical address mask (when INVALID
is passed in) or to activate the given logical address. So:

	if (addr == CEC_LOG_ADDR_INVALID) {
		hdmi_writel(dw_dev, 0x0, HDMI_CEC_ADDR_L);
		hdmi_writel(dw_dev, 0x0, HDMI_CEC_ADDR_H);
		return 0;
	}

	if (i >= 8) {
		tmp = hdmi_readl(dw_dev, HDMI_CEC_ADDR_H);
		tmp |= BIT(i - 8);
		hdmi_writel(dw_dev, tmp, HDMI_CEC_ADDR_H);
	} else {
		tmp = hdmi_readl(dw_dev, HDMI_CEC_ADDR_L);
		tmp |= BIT(i);
		hdmi_writel(dw_dev, tmp, HDMI_CEC_ADDR_L);
	}

That's all you need to do.

> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dw_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> +		u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = cec_get_drvdata(adap);
>>> +	u8 len = msg->len;
>>> +	u32 reg;
>>> +	int i;
>>> +
>>> +	if (len > HDMI_CEC_TX_DATA_MAX) {
>>> +		dev_err(dw_dev->dev, "%s: invalid len (%d)\n", __func__, len);
>>> +		return -EINVAL;
>>> +	}
>> Can't happen.
>>
>>> +
>>> +	if (hdmi_readl(dw_dev, HDMI_CEC_CTRL) & HDMI_CEC_CTRL_SEND_MASK) {
>>> +		dev_err(dw_dev->dev, "%s: tx is busy\n", __func__);
>>> +		return -EBUSY;
>> This should never happen, but it doesn't hurt to check.
>>
>>> +	}
>>> +
>>> +	for (i = 0; i < HDMI_CEC_TX_DATA_MAX; i++)
>>> +		hdmi_writel(dw_dev, 0x0, HDMI_CEC_TX_DATA(i));
>> Why clear this? Is it necessary?
>>
>>> +	for (i = 0; i < len; i++)
>>> +		hdmi_writel(dw_dev, msg->msg[i], HDMI_CEC_TX_DATA(i));
>>> +
>>> +	switch (signal_free_time) {
>>> +	case 3:
>>> +		reg = 0x0;
>>> +		break;
>>> +	case 7:
>>> +		reg = 0x2;
>>> +		break;
>>> +	case 5:
>> Use the defines in media/cec.h instead of hardcoding these values.
>>
>>> +	default:
>>> +		reg = 0x1;
>>> +		break;
>>> +	}
>>> +
>>> +	hdmi_writel(dw_dev, len, HDMI_CEC_TX_CNT);
>>> +	hdmi_mask_writel(dw_dev, reg, HDMI_CEC_CTRL,
>>> +			HDMI_CEC_CTRL_FRAME_TYP_OFFSET,
>>> +			HDMI_CEC_CTRL_FRAME_TYP_MASK);
>>> +	hdmi_mask_writel(dw_dev, 0x1, HDMI_CEC_CTRL,
>>> +			HDMI_CEC_CTRL_SEND_OFFSET,
>>> +			HDMI_CEC_CTRL_SEND_MASK);
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct cec_adap_ops dw_hdmi_cec_adap_ops = {
>>> +	.adap_enable = dw_hdmi_cec_adap_enable,
>>> +	.adap_log_addr = dw_hdmi_cec_adap_log_addr,
>>> +	.adap_transmit = dw_hdmi_cec_adap_transmit,
>>> +};
>> <snip>
>>
>>> +static int dw_hdmi_log_status(struct v4l2_subdev *sd)
>>> +{
>>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>>> +	struct v4l2_dv_timings timings;
>>> +
>>> +	v4l2_info(sd, "--- Chip configuration ---\n");
>>> +	v4l2_info(sd, "cfg_clk=%dMHz\n", dw_dev->cfg_clk);
>>> +	v4l2_info(sd, "phy_drv=%s, phy_jtag_addr=0x%x\n", dw_dev->phy_drv,
>>> +			dw_dev->phy_jtag_addr);
>>> +
>>> +	v4l2_info(sd, "--- Chip status ---\n");
>>> +	v4l2_info(sd, "selected_input=%d: signal=%d\n", dw_dev->selected_input,
>>> +			has_signal(dw_dev, dw_dev->selected_input));
>>> +	v4l2_info(sd, "configured_input=%d: signal=%d\n",
>>> +			dw_dev->configured_input,
>>> +			has_signal(dw_dev, dw_dev->configured_input));
>>> +
>>> +	v4l2_info(sd, "--- CEC status ---\n");
>>> +	if (dw_dev->cec_enabled_adap) {
>>> +		int i;
>>> +
>>> +		for (i = 0; i < HDMI_CEC_MAX_LOG_ADDRS; i++) {
>>> +			bool is_valid = dw_dev->cec_valid_addrs & (1 << i);
>>> +
>>> +			if (is_valid)
>>> +				v4l2_info(sd, "cec_logical_address=0x%x\n",
>>> +						dw_dev->cec_addr[i]);
>>> +		}
>>> +	}
>> I'm not sure how useful this is. The same information is available through
>> /sys/kernel/debug/cec/cecX/status.
>>
>> And just running cec-ctl gives it as well.
>>
>> Up to you, though.
>>
>>> +
>>> +	v4l2_info(sd, "--- Video status ---\n");
>>> +	v4l2_info(sd, "type=%s, color_depth=%dbits",
>>> +			hdmi_readl(dw_dev, HDMI_PDEC_STS) &
>>> +			HDMI_PDEC_STS_DVIDET ? "dvi" : "hdmi",
>>> +			dw_hdmi_get_colordepth(dw_dev));
>>> +
>>> +	v4l2_info(sd, "--- Video timings ---\n");
>>> +	if (dw_hdmi_query_dv_timings(sd, &timings))
>>> +		v4l2_info(sd, "No video detected\n");
>>> +	else
>>> +		v4l2_print_dv_timings(sd->name, "Detected format: ",
>>> +				&timings, true);
>>> +	v4l2_print_dv_timings(sd->name, "Configured format: ",
>>> +			&dw_dev->timings, true);
>>> +	return 0;
>>> +}
>>> +
>> <snip>
>>
>>> +	/* CEC */
>>> +	dw_dev->cec_adap = cec_allocate_adapter(&dw_hdmi_cec_adap_ops,
>>> +			dw_dev, dev_name(dev), CEC_CAP_TRANSMIT |
>>> +			CEC_CAP_PHYS_ADDR | CEC_CAP_LOG_ADDRS,
>> Add CEC_CAP_RC and CEC_CAP_PASSTHROUGH.
>>
>> I'm not sure about CEC_CAP_PHYS_ADDR. The problem here is that this driver
>> doesn't handle the EDID, but without that it doesn't know what physical
>> address to use.
>>
>> I wonder if the cec-notifier can be used for this, possibly with adaptations.
>> Relying on users to set the physical address is a last resort since it is very
>> painful to do so. cec-notifier was specifically designed to solve this.
> 
> Yes, EDID ROM is not integrated into the controller so I can't
> add the code. How exactly can I use cec-notifier here?

drivers/media/platform/sti/cec/stih-cec.c is a good example. The notifier is
called by drivers/gpu/drm/sti/sti_hdmi.c.

Regards,

	Hans

Powered by blists - more mailing lists