Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

On Fri, 13 Sept 2024 at 11:46, Sandor Yu [off-list ref] wrote:

quoted

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501
DP/HDMI driver

On Fri, Sep 06, 2024 at 02:50:08AM GMT, Sandor Yu wrote:

quoted

quoted

On Tue, Sep 03, 2024 at 06:07:25AM GMT, Sandor Yu wrote:

quoted

quoted

-----Original Message-----
From: dri-devel <redacted> On
Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <redacted>
Cc: dmitry.baryshkov@linaro.org; andrzej.hajda@intel.com;
neil.armstrong@linaro.org; Laurent Pinchart
[off-list ref]; jonas@kwiboo.se;
jernej.skrabec@gmail.com; airlied@gmail.com; daniel@ffwll.ch;
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
shawnguo@kernel.org; s.hauer@pengutronix.de;

festevam@gmail.com;

quoted

quoted

quoted

quoted

vkoul@kernel.org; dri-devel@lists.freedesktop.org;
devicetree@vger.kernel.org;
linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org;
kernel@pengutronix.de; dl-linux-imx [off-list ref]; Oliver
Brown [off-list ref]; alexander.stein@ew.tq-group.com;
sam@ravnborg.org
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add
MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I
have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

quoted

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+   u8 hpd = 0xf;
+
+   hpd = cdns_mhdp8501_read_hpd(mhdp);
+   if (hpd == 1)
+           return connector_status_connected;
+   else if (hpd == 0)
+           return connector_status_disconnected;
+
+   dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

quoted

quoted

quoted

quoted

quoted

+   return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+   struct cdns_mhdp8501_device *mhdp = container_of(work,
+                                                struct cdns_mhdp8501_device,
+                                                hotplug_work.work);
+   enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

quoted

quoted

quoted

+
+   drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+   if (status == connector_status_connected) {
+           /* Cable connected  */
+           DRM_INFO("HDMI/DP Cable Plug In\n");
+           enable_irq(mhdp->irq[IRQ_OUT]);
+   } else if (status == connector_status_disconnected) {
+           /* Cable Disconnected  */
+           DRM_INFO("HDMI/DP Cable Plug Out\n");
+           enable_irq(mhdp->irq[IRQ_IN]);
+   }
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

If you can't for some reason, the reason should be documented in
your

driver.

quoted

iMX8MQ have two HPD interrupters, one for plugout and the other
for plugin, because they could not be masked, so we have to enable
one and

disable the other.

quoted

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get
spurious interrupts?

They don't have status registers and cannot be masked. If they are not
disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it
constantly sends spurious interrupts when it's enabled?

Yes, this interrupt is different from a normal one; it's likely a design flaw.
For instance, the plugin interrupt is continuously generated as long as the cable is plugged in,
only stopping when the cable is unplugged.
That's why two interrupts are used to detect cable plugout and plugin separately.
If interrupts aren't used, the only option is polling.

I think I've seen such strange design on other platforms, level
interrupt for HPD, which needs to be disabled via disable_irq().

quoted

quoted

quoted

quoted

quoted

quoted

+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_init(mhdp->phy);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+           goto clk_disable;
+   }
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_set_mode(mhdp->phy, phy_mode);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to configure PHY: %d\n", ret);
+           goto clk_disable;
+   }

Why do you need a shared mutex between the phy and HDMI

controller?

quoted

quoted

quoted

Both PHY and HDMI controller could access to the HDMI firmware by
mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in
your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected
by its owns mutex. However, this mutex can only protect each mailbox
access within their respective drivers, and it cannot provide
protection for access between the HDMI and PHY drivers.

The PHY driver only provides two API functions, and these functions
are only called in the HDMI driver. Therefore, when accessing these
functions, we use a mutex to protect them. This ensures that mailbox
access is protected across different PHY and HDMI drivers.

It's really about abstraction. You're using a publicly defined API, and change
the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI
and PHY drivers included?

In the current code implementation, cdns-mhdp-helper.c isn't a standalone driver but rather a library.
It provides fundamental mailbox access functions and basic register read/write operations that rely on the mailbox.
These functions are highly reusable across MHDP8501 and MHDP8546 and can be leveraged by future MHDP versions.

However, most MHDP firmware interactions involve a sequence of mailbox accesses, including sending commands and receiving firmware responses.
These commands constitute a significant portion of all firmware interactions, encompassing operations like EDID reading and DP link training.
Unfortunately, these commands cannot be reused between MHDP8501 and MHDP8546.

Creating a dedicated mailbox driver with its own mutex would effectively address race conditions.
However, this would necessitate relocating all mailbox-related functions to this driver.
Including these non-reusable functions would defeat the purpose of code reuse.

To strike a balance between code reusability and race condition mitigation, adding mutexes to PHY access functions seems like a reasonable solution.

You seem to have two kinds of scenarios when talking to MHDP: just
cdns_mhdp_mailbox_send(), no response needed and then the
cdns_mhdp_mailbox_send() /  cdns_mhdp_mailbox_recv_header() /
cdns_mhdp_mailbox_recv_data() sequence. Extract those + the mutex
access to separate functions, add a mutex to those sequences and use
them as a high-level API for your HDMI and PHY drivers.

Adding mutexes around phy_foo() calls doesn't look like a proper
solution _at_ _all_.

Sandor

quoted

quoted

quoted

quoted

quoted

quoted

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+                          const struct drm_display_mode *mode,
+                          unsigned long long tmds_rate) {
+   struct cdns_mhdp8501_device *mhdp =

bridge->driver_private;

quoted

quoted

quoted

quoted

quoted

+   union phy_configure_opts phy_cfg;
+   int ret;
+
+   phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,

&phy_cfg);

quoted

quoted

quoted

quoted

quoted

+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret < 0)
+           return MODE_CLOCK_RANGE;
+
+   return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+                       const struct drm_display_info *info,
+                       const struct drm_display_mode *mode) {
+   unsigned long long tmds_rate;
+
+   /* We don't support double-clocked and Interlaced modes */
+   if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+       mode->flags & DRM_MODE_FLAG_INTERLACE)
+           return MODE_BAD;
+
+   /* MAX support pixel clock rate 594MHz */
+   if (mode->clock > 594000)
+           return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

This clock rate check is covered by function
tmds_char_rate_valid() It could be removed if keep function
tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

quoted

quoted

quoted

+   if (mode->hdisplay > 3840)
+           return MODE_BAD_HVALUE;
+
+   if (mode->vdisplay > 2160)
+           return MODE_BAD_VVALUE;
+
+   tmds_rate = mode->clock * 1000ULL;
+   return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
+tmds_rate);

It will already be called by the core so this is redundant.

mode_valid function is use to filter the mode list in
drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called,
unsupported

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm
not entirely sure how to reconcile drm_bridge_connector and the hdmi
framework there, but we should at the very least provide a
mode_valid helper for bridges.

I agree with that. In fact, I'm a bit confused about the current
mode_valid and tmds_char_rate_valid functions. Ideally, we should find
a way to make tmds_char_rate_valid also work for filtering out the
mode list, rather than just during atomic_check.

Yeah, definitely. The way we did so on vc4 for example was to compute the
rate for a 8bpc, RGB, output and try to validate that. I think it would be
reasonable to start with that.

+1, please extract this code as a helper. You can even submit it
separately, reworking sun4i and vc4 to use a new helper.

-- 
With best wishes
Dmitry