[EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Sep 24 09:38:26 UTC 2024
On Tue, Sep 24, 2024 at 07:04:58AM GMT, Sandor Yu wrote:
>
> >
> > On Fri, 13 Sept 2024 at 11:46, Sandor Yu <sandor.yu at nxp.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 at lists.freedesktop.org> On
> > > > > > > > Behalf Of Maxime Ripard
> > > > > > > > Sent: 2024年7月2日 21:25
> > > > > > > > To: Sandor Yu <sandor.yu at nxp.com>
> > > > > > > > Cc: dmitry.baryshkov at linaro.org; andrzej.hajda at intel.com;
> > > > > > > > neil.armstrong at linaro.org; Laurent Pinchart
> > > > > > > > <laurent.pinchart at ideasonboard.com>; jonas at kwiboo.se;
> > > > > > > > jernej.skrabec at gmail.com; airlied at gmail.com;
> > > > > > > > daniel at ffwll.ch;
> > > > > > > > robh+dt at kernel.org; krzysztof.kozlowski+dt at linaro.org;
> > > > > > > > shawnguo at kernel.org; s.hauer at pengutronix.de;
> > > > festevam at gmail.com;
> > > > > > > > vkoul at kernel.org; dri-devel at lists.freedesktop.org;
> > > > > > > > devicetree at vger.kernel.org;
> > > > > > > > linux-arm-kernel at lists.infradead.org;
> > > > > > > > linux-kernel at vger.kernel.org; linux-phy at lists.infradead.org;
> > > > > > > > kernel at pengutronix.de; dl-linux-imx <linux-imx at nxp.com>;
> > > > > > > > Oliver Brown <oliver.brown at nxp.com>;
> > > > > > > > alexander.stein at ew.tq-group.com; sam at 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:
> > > > > > > > > +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.
Why is it so? In the end, even if one driver uses it and another driver
uses other functions (while holding the mutex), please move generic
versions of those functions to your helper code. It should abstract
_types_ of hardware access (and send/recv_header/recv_data is such an
abstraction).
> 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.
--
With best wishes
Dmitry
More information about the dri-devel
mailing list