Re: [PATCH v4 2/4] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library

[Cristian Ciocaltea]

On 9/3/24 11:09 AM, Maxime Ripard wrote:
> On Tue, Sep 03, 2024 at 12:12:02AM GMT, Cristian Ciocaltea wrote:
>> On 9/2/24 10:36 AM, Maxime Ripard wrote:
>>> On Sat, Aug 31, 2024 at 01:21:48AM GMT, Cristian Ciocaltea wrote:
>>>> On 8/27/24 11:58 AM, Maxime Ripard wrote:
>>>>> On Mon, Aug 19, 2024 at 01:29:29AM GMT, Cristian Ciocaltea wrote:
>>>>>> +static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
>>>>>> +{
>>>>>> +	struct dw_hdmi_qp *hdmi = dev_id;
>>>>>> +	struct dw_hdmi_qp_i2c *i2c = hdmi->i2c;
>>>>>> +	u32 stat;
>>>>>> +
>>>>>> +	stat = dw_hdmi_qp_read(hdmi, MAINUNIT_1_INT_STATUS);
>>>>>> +
>>>>>> +	i2c->stat = stat & (I2CM_OP_DONE_IRQ | I2CM_READ_REQUEST_IRQ |
>>>>>> +			    I2CM_NACK_RCVD_IRQ);
>>>>>> +
>>>>>> +	if (i2c->stat) {
>>>>>> +		dw_hdmi_qp_write(hdmi, i2c->stat, MAINUNIT_1_INT_CLEAR);
>>>>>> +		complete(&i2c->cmp);
>>>>>> +	}
>>>>>> +
>>>>>> +	if (stat)
>>>>>> +		return IRQ_HANDLED;
>>>>>> +
>>>>>> +	return IRQ_NONE;
>>>>>> +}
>>>>>
>>>>> If the scrambler is enabled, you need to deal with hotplug. On hotplug,
>>>>> the monitor will drop its TMDS ratio and scrambling status, but the
>>>>> driver will keep assuming it's been programmed.
>>>>>
>>>>> If you don't have a way to deal with hotplug yet, then I'd suggest to
>>>>> just drop the scrambler setup for now.
>>>>
>>>> Thanks for the heads up!
>>>>
>>>> HPD is partially handled by the RK platform driver, which makes use of
>>>> drm_helper_hpd_irq_event(). Since the bridge sets DRM_BRIDGE_OP_DETECT
>>>> flag, the dw_hdmi_qp_bridge_detect() callback gets executed, which in turn
>>>> verifies the PHY status via ->read_hpd() implemented as
>>>> dw_hdmi_qp_rk3588_read_hpd() in the platform driver.
>>>
>>> It's not only about hotplug detection, it's also about what happens
>>> after you've detected a disconnection / reconnection.
>>>
>>> The framework expects to keep the current mode as is, despite the
>>> monitor not being setup to use the scrambler anymore, and the display
>>> remains black.
>>
>> AFAICS, the ->atomic_enable() callback is always invoked upon
>> reconnection, hence the scrambler gets properly (re)enabled via
>> dw_hdmi_qp_setup().
> 
> No, it's not.
> 
>>>> During my testing so far it worked reliably when switching displays with
>>>> different capabilities.  I don't have a 4K@60Hz display at the moment, but
>>>> used the HDMI RX port on the Rock 5B board in a loopback connection to
>>>> verify this mode, which triggered the high TMDS clock ratio and scrambling
>>>> setup as well.
>>>
>>> How did you test exactly?
>>
>> I initially tested with Sway/wlroots having an app running
>> (eglgears_wayland) while unplugging/replugging the HDMI connectors in
>> every possible sequence I could think of (e.g. several times per
>> display, switching to a different one, repeating, switching again, etc).
>>
>> I've just retested the whole stuff with Weston and confirm it works as
>> expected, i.e. no black screen (or bad capture stream for the 4K@60Hz
>> case) after any of the reconnections.
> 
> Then I guess both sway and weston handle uevent and will change the
> connector mode on reconnection.
> 
> It's not mandatory, and others will just not bother and still expect the
> output to work.
> 
> I guess the easier you can test this with is modetest.

Indeed, modetest doesn't trigger a mode change on reconnection.
This is handled now in v6:

https://lore.kernel.org/all/20240906-b4-rk3588-bridge-upstream-v6-0-a3128fb103eb@xxxxxxxxxxxxx/

Thanks,
Cristian