Re: [PATCH v3 06/10] media: synopsys: hdmirx: give the signal lock wait a real timeout

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 10/5/26 16:28, Sascha Hauer wrote:
> From: Gerald Loacker <gerald.loacker@wolfvision.net>
> 
> hdmirx_wait_signal_lock() bounded itself by a count of 300 iterations.
> How long that came to depended on the debounce inside
> tx_5v_power_present(), which the loop calls once per iteration and which
> sleeps about 7ms of it, so the effective timeout was around two seconds
> by accident rather than by design.
> 
> That accident is about to go away. A later patch in this series takes the
> 5V state from an upstream subdev where the board has no hpd-gpios, which
> leaves tx_5v_power_present() a plain read of a flag costing microseconds.
> The same 300 iterations would then be over in a few milliseconds and the
> wait would give up almost immediately.
> 
> Poll on a fixed 10ms interval against an explicit three second deadline.
> Check the deadline after the 5V test so a disconnect is still reported as
> -ENOLINK rather than a timeout, and report the timeout as -ETIMEDOUT; the
> sole caller only tests for non-zero, so neither is a change in behaviour.
> 
> Assisted-by: LLM
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   .../media/platform/synopsys/hdmirx/snps_hdmirx.c   | 40 +++++++++++++++-------
>   1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 6231bb28616c1..a28eb86e0b83c 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> @@ -2129,13 +2129,25 @@ static irqreturn_t hdmirx_dma_irq_handler(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>   
> +/*
> + * Wait for the HDMI signal to lock: TMDS clock detection and ratio
> + * configuration, then PHY and CMU lock, then DMA lock.
> + *
> + * The wait used to be bounded by a count of 300 iterations. What that came
> + * to in wall clock time was decided by the debounce inside
> + * tx_5v_power_present(), which is called once per iteration and sleeps
> + * about 7ms of it, so the real timeout was an accident of how 5V happens to
> + * be sensed. Poll on a fixed interval against an explicit deadline instead.
> + */
>   static int hdmirx_wait_signal_lock(struct snps_hdmirx_dev *hdmirx_dev)
>   {
>   	struct v4l2_device *v4l2_dev = &hdmirx_dev->v4l2_dev;
>   	u32 mu_status, scdc_status, dma_st10, cmu_st;
> -	u32 i;
> +	unsigned long timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(3000);
>   
> -	for (i = 0; i < 300; i++) {
> +	for (;;) {
>   		mu_status = hdmirx_readl(hdmirx_dev, MAINUNIT_STATUS);
>   		scdc_status = hdmirx_readl(hdmirx_dev, SCDC_REGBANK_STATUS3);
>   		dma_st10 = hdmirx_readl(hdmirx_dev, DMA_STATUS10);
> @@ -2149,21 +2161,25 @@ static int hdmirx_wait_signal_lock(struct snps_hdmirx_dev *hdmirx_dev)
>   		if (!tx_5v_power_present(hdmirx_dev)) {
>   			v4l2_dbg(1, debug, v4l2_dev,
>   				 "%s: HDMI pull out, return\n", __func__);
> -			return -1;
> +			return -ENOLINK;
>   		}
>   
> -		hdmirx_tmds_clk_ratio_config(hdmirx_dev);
> -	}
> +		if (time_after(jiffies, timeout)) {
> +			v4l2_err(v4l2_dev,
> +				 "%s: signal not lock, tmds_clk_ratio:%d\n",
> +				 __func__, hdmirx_dev->tmds_clk_ratio);
> +			v4l2_err(v4l2_dev,
> +				 "%s: mu_st:%#x, scdc_st:%#x, dma_st10:%#x, cmu_st:%#x\n",
> +				 __func__, mu_status, scdc_status, dma_st10,
> +				 cmu_st);
> +			return -ETIMEDOUT;
> +		}
>   
> -	if (i == 300) {
> -		v4l2_err(v4l2_dev, "%s: signal not lock, tmds_clk_ratio:%d\n",
> -			 __func__, hdmirx_dev->tmds_clk_ratio);
> -		v4l2_err(v4l2_dev, "%s: mu_st:%#x, scdc_st:%#x, dma_st10:%#x\n",
> -			 __func__, mu_status, scdc_status, dma_st10);
> -		return -1;
> +		hdmirx_tmds_clk_ratio_config(hdmirx_dev);
> +		usleep_range(10000, 11000);
>   	}
>   
> -	v4l2_dbg(1, debug, v4l2_dev, "%s: signal lock ok, i:%d\n", __func__, i);
> +	v4l2_dbg(1, debug, v4l2_dev, "%s: signal lock ok\n", __func__);
>   	hdmirx_writel(hdmirx_dev, GLOBAL_SWRESET_REQUEST, DATAPATH_SWRESETREQ);
>   
>   	reinit_completion(&hdmirx_dev->avi_pkt_rcv);
> 

Acked-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry