Re: [PATCH] media: hdmirx: snps, rk: Fix EDID injection with proper HPD toggle on RK3588

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 09/02/2026 07:16, Ross Cawston wrote:
> The existing VIDIOC_S_EDID implementation writes EDID data but does not
> properly trigger source renegotiation on RK3588 boards. This results in
> no visible change to the source device despite the ioctl succeeding.
> 
> Even re-plugging the HDMI cable does not trigger renegotiation on previous
> versions of the driver. This version reliably triggers renegotiation on
> EDID injection on tested hardware.
> 
> Fix by mirroring the vendor BSP behavior:
> - Disable HDMI and DMA IRQs to avoid races.
> - Simulate plugout if 5V power is present.
> - Toggle HPD low before write.
> - Schedule a delayed hotplug workqueue (1000ms) to re-enable HPD and
>   force renegotiation.
> 
> Bump WAIT_SIGNAL_LOCK_TIME from 300ms to 600ms to ensure lock.
> 
> This ensures custom EDIDs take effect reliably, allowing userspace to
> force specific resolutions/timings.
> 
> Tested on Orange Pi 5 Ultra and Radxa Rock 5B — sources now correctly
> re-detect and lock to the new EDID modes.
> 
> Signed-off-by: Ross Cawston <ross@r-sc.ca>
> 
> ---
>  drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c | 43 +++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index abc123..def456 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> @@ -88,6 +88,10 @@
>  	HDMIRX_NUM_RST,
>  };
>  
> +#define WAIT_SIGNAL_LOCK_TIME		600
> +#define NO_LOCK_CFG_RETRY_TIME		300
> +#define WAIT_LOCK_STABLE_TIME		20
> +
>  static const char *const pix_fmt_str[] = {
>  	"RGB888",
>  	"YUV422",
> @@ -620,6 +624,7 @@
>  
>  	hdmirx_update_bits(hdmirx_dev, SCDC_CONFIG, POWERPROVIDED, 0);
>  	hdmirx_interrupts_setup(hdmirx_dev, false);
> +	hdmirx_hpd_ctrl(hdmirx_dev, false);
>  	hdmirx_update_bits(hdmirx_dev, DMA_CONFIG6, HDMIRX_DMA_EN, 0);
>  	hdmirx_update_bits(hdmirx_dev, DMA_CONFIG4,
>  			   LINE_FLAG_INT_EN |
> @@ -680,16 +685,32 @@
>  	 */
>  	mutex_lock(&hdmirx_dev->work_lock);
>  
> +	/*
> +	 * Some sources won't re-read EDID unless we avoid IRQ races and
> +	 * force a full plugout/HPD low sequence. On several tested devices,
> +	 * leaving IRQs enabled or skipping plugout kept the old EDID cached.
> +	 */

This is weird. There seem to be two (or more) different issues here: IRQ races
and HPD low->high toggle.

When updating the EDID the HPD must go low before updating the EDID, and
go high at least 100 ms after it went low. Experience shows that 'HZ / 7'
is a good value for HDMI receivers. 1000 ms as you are using below is overkill.

So the HPD toggle is required, otherwise video sources won't re-read the EDID.

The IRQ races must be something else, I'm not sure what.

I would recommend splitting this up: first add the HPD toggle since without it
video sources will not detect the EDID update at all.

Then follow up with other patches fixing whatever else is needed to make it
work reliable. Video sources will know nothing about enabling/disabling the
IRQ, so if that matters, then it is more likely that somehow keeping the IRQs
interferes with updating the EDID.

I think you need to better understand what is going on here :-)

Regards,

	Hans

> +	disable_irq(hdmirx_dev->hdmi_irq);
> +	disable_irq(hdmirx_dev->dma_irq);
> +
> +	if (tx_5v_power_present(hdmirx_dev))
> +		hdmirx_plugout(hdmirx_dev);
> +
>  	hdmirx_hpd_ctrl(hdmirx_dev, false);
>  
>  	if (edid->blocks) {
>  		hdmirx_write_edid(hdmirx_dev, edid);
> -		hdmirx_hpd_ctrl(hdmirx_dev, true);
>  	} else {
>  		cec_phys_addr_invalidate(hdmirx_dev->cec->adap);
>  		hdmirx_dev->edid_blocks_written = 0;
>  	}
>  
> +	enable_irq(hdmirx_dev->dma_irq);
> +	enable_irq(hdmirx_dev->hdmi_irq);
> +
> +	queue_delayed_work(system_unbound_wq, &hdmirx_dev->delayed_work_hotplug,
> +			   msecs_to_jiffies(1000));
> +
>  	mutex_unlock(&hdmirx_dev->work_lock);
>  
>  	return 0;
> @@ -2082,9 +2103,9 @@
>  {
>  	struct v4l2_device *v4l2_dev = &hdmirx_dev->v4l2_dev;
>  	u32 mu_status, scdc_status, dma_st10, cmu_st;
> -	u32 i;
> +	u32 i, j = 0;
>  
> -	for (i = 0; i < 300; i++) {
> +	for (i = 1; i < WAIT_SIGNAL_LOCK_TIME; i++) {
>  		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);
> @@ -2093,8 +2114,16 @@
>  		if ((mu_status & TMDSVALID_STABLE_ST) &&
>  		    (dma_st10 & HDMIRX_LOCK) &&
>  		    (cmu_st & TMDSQPCLK_LOCKED_ST))
> +			j++;
> +		else
> +			j = 0;
> +
> +		if (j > WAIT_LOCK_STABLE_TIME)
>  			break;
>  
> +		if (i % NO_LOCK_CFG_RETRY_TIME == 0)
> +			hdmirx_phy_config(hdmirx_dev);
> +
>  		if (!tx_5v_power_present(hdmirx_dev)) {
>  			v4l2_dbg(1, debug, v4l2_dev,
>  				 "%s: HDMI pull out, return\n", __func__);
> @@ -2104,7 +2133,7 @@
>  		hdmirx_tmds_clk_ratio_config(hdmirx_dev);
>  	}
>  
> -	if (i == 300) {
> +	if (i == WAIT_SIGNAL_LOCK_TIME) {
>  		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",
> @@ -2127,7 +2156,8 @@
>  				   PKTDEC_AVIIF_RCV_IRQ, 0);
>  	}
>  
> -	msleep(50);
> +	hdmirx_reset_dma(hdmirx_dev);
> +	msleep(500);
>  	hdmirx_format_change(hdmirx_dev);
>  
>  	return 0;
> @@ -2141,6 +2171,7 @@
>  	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);
>  	hdmirx_interrupts_setup(hdmirx_dev, true);
>  
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip