Message-ID:
 <PAXPR04MB94481627922756E892C37B7DF4682@PAXPR04MB9448.eurprd04.prod.outlook.com>
Date: Tue, 24 Sep 2024 07:04:58 +0000
From: Sandor Yu <sandor.yu@....com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Maxime Ripard <mripard@...nel.org>, "andrzej.hajda@...el.com"
	<andrzej.hajda@...el.com>, "neil.armstrong@...aro.org"
	<neil.armstrong@...aro.org>, Laurent Pinchart
	<laurent.pinchart@...asonboard.com>, "jonas@...boo.se" <jonas@...boo.se>,
	"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>, "airlied@...il.com"
	<airlied@...il.com>, "daniel@...ll.ch" <daniel@...ll.ch>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "s.hauer@...gutronix.de"
	<s.hauer@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>,
	"vkoul@...nel.org" <vkoul@...nel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "kernel@...gutronix.de"
	<kernel@...gutronix.de>, dl-linux-imx <linux-imx@....com>, Oliver Brown
	<oliver.brown@....com>, "alexander.stein@...tq-group.com"
	<alexander.stein@...tq-group.com>, "sam@...nborg.org" <sam@...nborg.org>
Subject: 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 <sandor.yu@....com> wrote:
> >
> >
> > > 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:
> > > > > On Tue, Sep 03, 2024 at 06:07:25AM GMT, Sandor Yu wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On
> > > > > > > Behalf Of Maxime Ripard
> > > > > > > Sent: 2024年7月2日 21:25
> > > > > > > To: Sandor Yu <sandor.yu@....com>
> > > > > > > Cc: dmitry.baryshkov@...aro.org; andrzej.hajda@...el.com;
> > > > > > > neil.armstrong@...aro.org; Laurent Pinchart
> > > > > > > <laurent.pinchart@...asonboard.com>; jonas@...boo.se;
> > > > > > > jernej.skrabec@...il.com; airlied@...il.com;
> > > > > > > daniel@...ll.ch;
> > > > > > > robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> > > > > > > shawnguo@...nel.org; s.hauer@...gutronix.de;
> > > festevam@...il.com;
> > > > > > > vkoul@...nel.org; dri-devel@...ts.freedesktop.org;
> > > > > > > devicetree@...r.kernel.org;
> > > > > > > linux-arm-kernel@...ts.infradead.org;
> > > > > > > linux-kernel@...r.kernel.org; linux-phy@...ts.infradead.org;
> > > > > > > kernel@...gutronix.de; dl-linux-imx <linux-imx@....com>;
> > > > > > > Oliver Brown <oliver.brown@....com>;
> > > > > > > alexander.stein@...tq-group.com; sam@...nborg.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:
> > > > > > > > +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);
> > > > > > > > +   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);
> > > > > > > > +
> > > > > > > > +   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.
> > > > > >
> > > > > > 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.
> > > > > > 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().
> 
> >
> > >
> > > > > > > > +   /* 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?
> > > > > >
> > > > > > 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_.
> 
Because the sequence cdns_mhdp_mailbox_send() / cdns_mhdp_mailbox_recv_header() / cdns_mhdp_mailbox_recv_data() cannot be reused by different drivers, 
it's not suitable to abstract them into a separate function.
I've noticed some Linux kernel drivers use global mutexes, which can solve the current race conditions problem. 
I'll implement this in the next version.

> >
> > Sandor
> >
> > > > > > >
> > > > > > > > +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;
> > > > > > > > +   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);
> > > > > > > > +   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.
> > > > >
> > > > > > >
> > > > > > > > +   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.
> 
OK, I would appreciate more time to carefully consider the implementation of this API.

B.R
Sandor

> --
> With best wishes
> Dmitry

Powered by blists - more mailing lists