Message-ID: <f188dd6f-c98e-4a70-a1f0-3d205b924b78@xs4all.nl>
Date: Thu, 20 Feb 2025 08:03:58 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Dmitry Osipenko <dmitry.osipenko@...labora.com>,
 Shreeya Patel <shreeya.patel@...labora.com>, Heiko Stuebner
 <heiko@...ech.de>, Mauro Carvalho Chehab <mchehab@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, jose.abreu@...opsys.com,
 nelson.costa@...opsys.com, shawn.wen@...k-chips.com,
 nicolas.dufresne@...labora.com,
 Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: kernel@...labora.com, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, Tim Surber <me@...surber.de>
Subject: Re: [PATCH v7 4/6] media: platform: synopsys: Add support for HDMI
 input driver

Hi Dmitry,

On 19/02/2025 19:35, Dmitry Osipenko wrote:
> Hi Hans,
> 
> Thank you very much for the review!
> 
> On 2/19/25 12:11, Hans Verkuil wrote:
>> Hi Dmitry,
>>
>> More review comments. Some are trivial, but the real problem is that the 5V/HPD
>> handling is really confusing and, from what I can tell, incorrect in places.
>>
>> I suspect that there are some misunderstandings on how this is supposed to
>> work. I've tried to explain it in my comments, but feel free to contact me if
>> anything is still unclear.
>>
> ...
>>> +#define EDID_NUM_BLOCKS_MAX				2
>>
>> Is this indeed the maximum number of supported EDID blocks?
>> These days 4 EDID blocks is typically the maximum. If the hardware
>> supports it, then I recommend implementing support for 4 EDID blocks.
>>
>> If it really only supports 2 blocks, then add a comment stating that
>> this is a HW limitation.
> 
> Good catch. Hardware supports 4 EDID blocks, will change in v8.

Ah, good. Make sure you test this, both writing 4 blocks to it and read it
from a transmitter. With v4l2-ctl --set-edid type=displayport-with-cta861
sets 3 blocks, and type=hdmi-4k-600mhz-with-displayid sets 4 blocks. So test
with both variants.

> 
> ...
>>> +static int hdmirx_subscribe_event(struct v4l2_fh *fh,
>>> +				  const struct v4l2_event_subscription *sub)
>>> +{
>>> +	switch (sub->type) {
>>> +	case V4L2_EVENT_SOURCE_CHANGE:
>>> +		if (fh->vdev->vfl_dir == VFL_DIR_RX)
>>
>> This is weird, the direction is always RX, so can't you just drop this test?
> 
> Suppose this code was copied from another driver, will drop.
> 
> ...
>>> +static int hdmirx_set_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>>> +{
>>> +	struct hdmirx_stream *stream = video_drvdata(file);
>>> +	struct snps_hdmirx_dev *hdmirx_dev = stream->hdmirx_dev;
>>> +	u16 phys_addr;
>>> +	int ret = 0;
>>> +
>>> +	if (edid->pad)
>>> +		return -EINVAL;
>>> +
>>> +	if (edid->start_block)
>>> +		return -EINVAL;
>>> +
>>> +	if (edid->blocks > EDID_NUM_BLOCKS_MAX) {
>>> +		edid->blocks = EDID_NUM_BLOCKS_MAX;
>>> +		return -E2BIG;
>>> +	}
>>> +
>>> +	hdmirx_disable_irq(hdmirx_dev->dev);
>>> +
>>> +	if (!edid->blocks) {
>>> +		cec_phys_addr_invalidate(hdmirx_dev->cec->adap);
>>> +		hdmirx_dev->edid_blocks_written = 0;
>>> +
>>> +		hdmirx_dev->hpd_pull_low = true;
>>> +
>>> +		if (tx_5v_power_present(hdmirx_dev))
>>> +			hdmirx_plugout(hdmirx_dev);
>>> +		else
>>> +			hdmirx_hpd_ctrl(hdmirx_dev, false);
>>> +	} else {
>>> +		phys_addr = cec_get_edid_phys_addr(edid->edid,
>>> +						   edid->blocks * 128, NULL);
>>> +		ret = v4l2_phys_addr_validate(phys_addr, &phys_addr, NULL);
>>> +		if (ret)
>>> +			goto out;
>>> +
>>> +		if (tx_5v_power_present(hdmirx_dev))
>>> +			hdmirx_plugout(hdmirx_dev);
>>> +
>>> +		hdmirx_dev->hpd_pull_low = false;
>>> +		hdmirx_hpd_ctrl(hdmirx_dev, false);
>>> +		hdmirx_write_edid(hdmirx_dev, edid);
>>> +	}
>>> +out:
>>> +	hdmirx_enable_irq(hdmirx_dev->dev);
>>
>> The way the HPD is handled is really confusing in the code. I had to dig through
>> the driver code to discover that the HPD is enabled via hdmirx_enable_irq(). But
>> normally interrupts have nothing to do with the HPD.
>>
>> The HPD is really only controlled by whether there is an EDID or not, and optionally
>> whether 5V is high or not. The only reason for pulling the HPD low if 5V is low is
>> to save a bit of power: if nothing is connected, then there is no need to pull the HPD
>> high, after all, nobody is listening to it.
>>
>> But this is typically entirely separate from interrupts.
>>
>> The 5V decides whether there is a video source or not, so if it goes low, then you
>> stop streaming. The HPD tells the video source if there is an EDID or not. But that
>> is independent of video streaming. Updating the EDID while the source is sending
>> video shouldn't interrupt video capture. Most likely the source will detect an EDID
>> change, parse the new EDID and then it might decide to stop streaming and reconfigure,
>> or just continue streaming.
>>
>> The code feels like you are trying to be smart, when it is really just a fairly
>> dumb mechanism.
>>
>> I think this should be rewritten, unless there are some odd hardware constraints.
>> In which case that should be documented.
> 
> Thanks a lot for the clarification! The HPD logic was borrowed from the downstream driver, will try to change it for v8. I'm not aware about hardware constraints and TRM suggests that driving HPD seprately from 5v should work fine, will see how it will work in practice.
> 
> ...
>>> +static int hdmirx_g_parm(struct file *file, void *priv,
>>> +			 struct v4l2_streamparm *parm)
>>> +{
>>> +	struct hdmirx_stream *stream = video_drvdata(file);
>>> +	struct snps_hdmirx_dev *hdmirx_dev = stream->hdmirx_dev;
>>> +	struct v4l2_fract fps;
>>> +
>>> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>>> +		return -EINVAL;
>>> +
>>> +	fps = v4l2_calc_timeperframe(&hdmirx_dev->timings);
>>> +	parm->parm.capture.timeperframe.numerator = fps.numerator;
>>> +	parm->parm.capture.timeperframe.denominator = fps.denominator;
>>
>> You can just write:
>>
>> 	parm->parm.capture.timeperframe = v4l2_calc_timeperframe(&hdmirx_dev->timings);
> 
> Ack
> 
> ...
>>> +static void hdmirx_plugin(struct snps_hdmirx_dev *hdmirx_dev)
>>> +{
>>> +	hdmirx_submodule_init(hdmirx_dev);
>>> +	hdmirx_update_bits(hdmirx_dev, SCDC_CONFIG, POWERPROVIDED,
>>> +			   POWERPROVIDED);
>>> +	hdmirx_hpd_ctrl(hdmirx_dev, true);
>>
>> Just because the 5V appeared, it doesn't mean that the HPD should
>> go high. That depends on whether there is an EDID or not.
>>
>> As I said above, the whole 5V/HPD handling seems confused in this driver.
> 
> In v6 I added 'hpd_pull_low' variable that force-disables HPD if there is no EDID. Hence, this hdmirx_hpd_ctrl() call doesn't enable HPD without EDID. I'll remove this call in v8 if driving HPD independently from 5v status will work fine.
> 
> static void hdmirx_hpd_ctrl(struct snps_hdmirx_dev *hdmirx_dev, bool en)
> {
> ...
> 	if (hdmirx_dev->hpd_pull_low && en)
> 		return;
>         ^^^^
> 
> ...
>>> +static void hdmirx_delayed_work_res_change(struct work_struct *work)
>>> +{
>>> +	struct snps_hdmirx_dev *hdmirx_dev;
>>> +	bool plugin;
>>> +
>>> +	hdmirx_dev = container_of(work, struct snps_hdmirx_dev,
>>> +				  delayed_work_res_change.work);
>>> +
>>> +	mutex_lock(&hdmirx_dev->work_lock);
>>> +	plugin = tx_5v_power_present(hdmirx_dev);
>>> +	v4l2_dbg(1, debug, &hdmirx_dev->v4l2_dev, "%s: plugin:%d\n",
>>> +		 __func__, plugin);
>>> +	if (plugin) {
>>> +		hdmirx_interrupts_setup(hdmirx_dev, false);
>>> +		hdmirx_submodule_init(hdmirx_dev);
>>> +		hdmirx_update_bits(hdmirx_dev, SCDC_CONFIG, POWERPROVIDED,
>>> +				   POWERPROVIDED);
>>> +		hdmirx_hpd_ctrl(hdmirx_dev, true);
>>> +		hdmirx_phy_config(hdmirx_dev);
>>> +
>>> +		if (hdmirx_wait_signal_lock(hdmirx_dev)) {
>>> +			hdmirx_plugout(hdmirx_dev);
>>
>> plugout() pulls the HPD low, but why? That has nothing to do with the signal lock.
>>
>> You need to take a good look at the 5V/HPD handling, this isn't right.
>>
>> Feel free to ask questions if you are not clear on how it should behave.
> 
> Again thanks a lot for the review, very appreciate! Will contact you if will become necessary.
> 

Regards,

	Hans

Powered by blists - more mailing lists