LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jose Abreu <Jose.Abreu@synopsys.com>
To: Hans Verkuil <hverkuil@xs4all.nl>, <linux-media@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	"Sylwester Nawrocki" <snawrocki@kernel.org>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	Philippe Ombredanne <pombredanne@nexb.com>
Subject: Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver
Date: Tue, 12 Dec 2017 16:02:04 +0000	[thread overview]
Message-ID: <635e7d70-0edb-7506-c268-9ebbae1eb39e@synopsys.com> (raw)
In-Reply-To: <108e2c3c-243f-cd67-2df7-57541b28ca39@xs4all.nl>

Hi Hans,

On 12-12-2017 15:47, Hans Verkuil wrote:
> Hi Jose,
>
> Some more comments:

Thanks for the review!

>
> On 11/12/17 18:41, 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@60Hz
>> 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@synopsys.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Philippe Ombredanne <pombredanne@nexb.com>
>> ---
>> Changes from v9:
>> 	- Use SPDX License ID (Philippe)
>> 	- Use LOW_DRIVE CEC error (Hans)
>> 	- Fill bt->il_* fields (Hans)
>> 	- Fix format->field to NONE (Hans)
>> 	- Drop a left-over comment (Hans)
>> 	- Use CEC_CAP_DEFAULTS (Hans)
>> Changes from v8:
>> 	- Incorporate Sakari's work on ASYNC subdevs
>> Changes from v6:
>> 	- edid-phandle now also looks for parent node (Sylwester)
>> 	- Fix kbuild build warnings
>> Changes from v5:
>> 	- Removed HDCP 1.4 support (Hans)
>> 	- Removed some CEC debug messages (Hans)
>> 	- Add s_dv_timings callback (Hans)
>> 	- Add V4L2_CID_DV_RX_POWER_PRESENT ctrl (Hans)
>> Changes from v4:
>> 	- Add flag V4L2_SUBDEV_FL_HAS_DEVNODE (Sylwester)
>> 	- Remove some comments and change some messages to dev_dbg (Sylwester)
>> 	- Use v4l2_async_subnotifier_register() (Sylwester)
>> Changes from v3:
>> 	- Use v4l2 async API (Sylwester)
>> 	- Do not block waiting for phy
>> 	- Do not use busy waiting delays (Sylwester)
>> 	- Simplify dw_hdmi_power_on (Sylwester)
>> 	- Use clock API (Sylwester)
>> 	- Use compatible string (Sylwester)
>> 	- Minor fixes (Sylwester)
>> Changes from v2:
>> 	- Address review comments from Hans regarding CEC
>> 	- Use CEC notifier
>> 	- Enable SCDC
>> Changes from v1:
>> 	- Add support for CEC
>> 	- Correct typo errors
>> 	- Correctly detect interlaced video modes
>> 	- Correct VIC parsing
>> Changes from RFC:
>> 	- Add support for HDCP 1.4
>> 	- Fixup HDMI_VIC not being parsed (Hans)
>> 	- Send source change signal when powering off (Hans)
>> 	- Add a "wait stable delay"
>> 	- Detect interlaced video modes (Hans)
>> 	- Restrain g/s_register from reading/writing to HDCP regs (Hans)
>> ---
>>  drivers/media/platform/dwc/Kconfig      |   15 +
>>  drivers/media/platform/dwc/Makefile     |    1 +
>>  drivers/media/platform/dwc/dw-hdmi-rx.c | 1840 +++++++++++++++++++++++++++++++
>>  drivers/media/platform/dwc/dw-hdmi-rx.h |  419 +++++++
>>  include/media/dwc/dw-hdmi-rx-pdata.h    |   48 +
>>  5 files changed, 2323 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..3ddccde 100644
>> --- a/drivers/media/platform/dwc/Kconfig
>> +++ b/drivers/media/platform/dwc/Kconfig
>> @@ -6,3 +6,18 @@ 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
>> +	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.
>> +
>> +config VIDEO_DWC_HDMI_RX_CEC
>> +	bool
>> +	depends on VIDEO_DWC_HDMI_RX
>> +	select CEC_CORE
>> +	select CEC_NOTIFIER
>> diff --git a/drivers/media/platform/dwc/Makefile b/drivers/media/platform/dwc/Makefile
>> index fc3b62c..cd04ca9 100644
>> --- a/drivers/media/platform/dwc/Makefile
>> +++ b/drivers/media/platform/dwc/Makefile
>> @@ -1 +1,2 @@
>>  obj-$(CONFIG_VIDEO_DWC_HDMI_PHY_E405) += dw-hdmi-phy-e405.o
>> +obj-$(CONFIG_VIDEO_DWC_HDMI_RX) += dw-hdmi-rx.o
>> diff --git a/drivers/media/platform/dwc/dw-hdmi-rx.c b/drivers/media/platform/dwc/dw-hdmi-rx.c
>> new file mode 100644
>> index 0000000..437351e
>> --- /dev/null
>> +++ b/drivers/media/platform/dwc/dw-hdmi-rx.c
> <snip>
>
>> +static int dw_hdmi_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
>> +		u32 config)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	if (!has_signal(dw_dev, input))
>> +		return -EINVAL;
> Why would this be a reason to reject this? There may be no signal now, but a signal
> might appear later.

I would expect s_routing to only be called if there is an input
connected, otherwise we are just wasting resources by trying to
equalize an input that is not present ... I can remove the "if"
as there are other safe guards for this though (for example g_fmt
will return an error) ...

>
>> +
>> +	dw_dev->selected_input = input;
>> +	if (input == dw_dev->configured_input)
>> +		return 0;
>> +
>> +	dw_hdmi_power_off(dw_dev);
>> +	return dw_hdmi_power_on(dw_dev, input);
>> +}
>> +
>> +static int dw_hdmi_g_input_status(struct v4l2_subdev *sd, u32 *status)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	*status = 0;
>> +	if (!has_signal(dw_dev, dw_dev->selected_input))
>> +		*status |= V4L2_IN_ST_NO_POWER;
>> +	if (is_off(dw_dev))
>> +		*status |= V4L2_IN_ST_NO_SIGNAL;
>> +
>> +	dev_dbg(dw_dev->dev, "%s: status=0x%x\n", __func__, *status);
>> +	return 0;
>> +}
>> +
>> +static int dw_hdmi_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parm)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	dev_dbg(dw_dev->dev, "%s\n", __func__);
>> +
>> +	/* TODO: Use helper to compute timeperframe */
>> +	parm->parm.capture.timeperframe.numerator = 1;
>> +	parm->parm.capture.timeperframe.denominator = 60;
>> +	return 0;
>> +}
>> +
>> +static int dw_hdmi_s_dv_timings(struct v4l2_subdev *sd,
>> +		struct v4l2_dv_timings *timings)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	dev_dbg(dw_dev->dev, "%s\n", __func__);
>> +	if (!v4l2_valid_dv_timings(timings, &dw_hdmi_timings_cap, NULL, NULL))
>> +		return -EINVAL;
>> +	if (v4l2_match_dv_timings(timings, &dw_dev->timings, 0, false))
>> +		return 0;
>> +
>> +	dw_dev->timings = *timings;
>> +	return 0;
>> +}
>> +
>> +static int dw_hdmi_g_dv_timings(struct v4l2_subdev *sd,
>> +		struct v4l2_dv_timings *timings)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	dev_dbg(dw_dev->dev, "%s\n", __func__);
>> +
>> +	*timings = dw_dev->timings;
>> +	return 0;
>> +}
>> +
>> +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;
>> +	bool is_hdmi_vic;
>> +	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 = hdmi_readl(dw_dev, HDMI_MD_STS) & HDMI_MD_STS_ILACE ?
>> +		V4L2_DV_INTERLACED : V4L2_DV_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);
>> +
>> +	/* HTOT = HACT + HFRONT + HSYNC + HBACK */
>> +	htot = hdmi_mask_readl(dw_dev, HDMI_MD_HT1,
>> +			HDMI_MD_HT1_HTOT_PIX_OFFSET,
>> +			HDMI_MD_HT1_HTOT_PIX_MASK);
>> +	/* HOFS = HSYNC + HBACK */
>> +	hofs = hdmi_mask_readl(dw_dev, HDMI_MD_HT1,
>> +			HDMI_MD_HT1_HOFS_PIX_OFFSET,
>> +			HDMI_MD_HT1_HOFS_PIX_MASK);
>> +
>> +	bt->hfrontporch = htot - hofs - bt->width;
>> +	bt->hsync = hdmi_mask_readl(dw_dev, HDMI_MD_HT0,
>> +			HDMI_MD_HT0_HS_CLK_OFFSET,
>> +			HDMI_MD_HT0_HS_CLK_MASK);
>> +	bt->hbackporch = hofs - bt->hsync;
>> +
>> +	/* VTOT = VACT + VFRONT + VSYNC + VBACK */
>> +	vtot = hdmi_readl(dw_dev, HDMI_MD_VTL);
>> +
>> +	hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL,
>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>> +	msleep(50); /* Wait for 1 field */
> How do you know this waits for 1 field? Or is this: "Wait for at least 1 field"?

Its wait at least for 1 field. This is over-generous because its
assuming the frame rate is 20fps (which in HDMI does not happen).

> I don't know exactly how the IP does this, but it looks fishy to me. If it is
> correct, then it could use a few comments about what is going on here as it is
> not obvious.

The IP updates the values at each field but I need to change this
register to populate all values in the bt struct.

>
> And what happens if the framerate is even slower? You know the pixelclock and
> total width+height, so you can calculate the framerate from that.

Hmm, but then I have to consider pixelclk error, msleep error, ...

>
>> +	bt->vsync = hdmi_readl(dw_dev, HDMI_MD_VOL);
>> +
>> +	hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>> +			HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>> +	msleep(50); /* Wait for 1 field */
>> +	bt->vbackporch = hdmi_readl(dw_dev, HDMI_MD_VOL);
>> +	bt->vfrontporch = vtot - bt->height - bt->vsync - bt->vbackporch;
>> +
>> +	if (bt->interlaced == V4L2_DV_INTERLACED) {
>> +		hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL,
>> +				HDMI_MD_VCTRL_V_MODE_OFFSET,
>> +				HDMI_MD_VCTRL_V_MODE_MASK);
>> +		msleep(100); /* Wait for 2 fields */
>> +
>> +		vtot = hdmi_readl(dw_dev, HDMI_MD_VTL);
>> +		hdmi_mask_writel(dw_dev, 0x1, HDMI_MD_VCTRL,
>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>> +		msleep(50); /* Wait for 1 field */
>> +		bt->il_vsync = hdmi_readl(dw_dev, HDMI_MD_VOL);
>> +
>> +		hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_OFFSET,
>> +				HDMI_MD_VCTRL_V_OFFS_LIN_MODE_MASK);
>> +		msleep(50);
>> +		bt->il_vbackporch = hdmi_readl(dw_dev, HDMI_MD_VOL);
>> +		bt->il_vfrontporch = vtot - bt->height - bt->il_vsync -
>> +			bt->il_vbackporch;
>> +
>> +		hdmi_mask_writel(dw_dev, 0x0, HDMI_MD_VCTRL,
>> +				HDMI_MD_VCTRL_V_MODE_OFFSET,
>> +				HDMI_MD_VCTRL_V_MODE_MASK);
> Same here, I'm not sure this is correct. What is the output of
> 'v4l2-ctl --query-dv-timings' when you feed it a standard interlaced format?

I used v4l2-ctl --log-status with interlaced format and
everything seemed correct ...

Thanks and Best Regards,
Jose Miguel Abreu

>
>> +	}
>> +
>> +	bt->standards = V4L2_DV_BT_STD_CEA861;
>> +
>> +	vic = dw_hdmi_get_curr_vic(dw_dev, &is_hdmi_vic);
>> +	if (vic) {
>> +		if (is_hdmi_vic) {
>> +			bt->flags |= V4L2_DV_FL_HAS_HDMI_VIC;
>> +			bt->hdmi_vic = vic;
>> +			bt->cea861_vic = 0;
>> +		} else {
>> +			bt->flags |= V4L2_DV_FL_HAS_CEA861_VIC;
>> +			bt->hdmi_vic = 0;
>> +			bt->cea861_vic = vic;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> Regards,
>
> 	Hans
>

  reply	other threads:[~2017-12-12 16:02 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-11 17:41 [PATCH v10 0/4] Synopsys DesignWare HDMI Video Capture Controller + PHY Jose Abreu
2017-12-11 17:41 ` [PATCH v10 1/4] dt-bindings: media: Document Synopsys DesignWare HDMI RX Jose Abreu
2017-12-15 13:34   ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 2/4] MAINTAINERS: Add entry for Synopsys DesignWare HDMI drivers Jose Abreu
2017-12-15 13:35   ` Hans Verkuil
2017-12-15 13:35     ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 3/4] [media] platform: Add Synopsys DesignWare HDMI RX PHY e405 Driver Jose Abreu
2017-12-11 19:05   ` Philippe Ombredanne
2017-12-15 13:46   ` Hans Verkuil
2017-12-11 17:41 ` [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver Jose Abreu
2017-12-11 19:06   ` Philippe Ombredanne
2017-12-12 15:47   ` Hans Verkuil
2017-12-12 16:02     ` Jose Abreu [this message]
2017-12-13 10:00       ` Hans Verkuil
2017-12-13 10:04         ` Philippe Ombredanne
2017-12-13 14:00         ` Jose Abreu
2017-12-13 20:49           ` Hans Verkuil
2017-12-15 11:23             ` Jose Abreu
2018-06-08  7:41               ` Hans Verkuil
2018-06-12 13:45                 ` Jose Abreu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=635e7d70-0edb-7506-c268-9ebbae1eb39e@synopsys.com \
    --to=jose.abreu@synopsys.com \
    --cc=Joao.Pinto@synopsys.com \
    --cc=hans.verkuil@cisco.com \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=pombredanne@nexb.com \
    --cc=sakari.ailus@iki.fi \
    --cc=snawrocki@kernel.org \
    --subject='Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).