Message-ID: <20240924-miniature-jade-gibbon-c9a226@houat>
Date: Tue, 24 Sep 2024 11:51:27 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Sandor Yu <sandor.yu@....com>
Cc: "dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.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, Sep 13, 2024 at 09:46:12AM GMT, Sandor Yu 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.

Urgh. Ok. At least, you should document that in your code then.

> > > > > > > +	/* 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.

No, it's really not. You need to address this in some other way. Why
can't you put the mutex in cdns-mhdp-helper.c calls?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists