> > On Fri, 13 Sept 2024 at 11:46, Sandor Yu <sandor.yu@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> On > > > > > > > Behalf Of Maxime Ripard > > > > > > > Sent: 2024年7月2日 21:25 > > > > > > > To: Sandor Yu <sandor.yu@xxxxxxx> > > > > > > > Cc: dmitry.baryshkov@xxxxxxxxxx; andrzej.hajda@xxxxxxxxx; > > > > > > > neil.armstrong@xxxxxxxxxx; Laurent Pinchart > > > > > > > <laurent.pinchart@xxxxxxxxxxxxxxxx>; jonas@xxxxxxxxx; > > > > > > > jernej.skrabec@xxxxxxxxx; airlied@xxxxxxxxx; > > > > > > > daniel@xxxxxxxx; > > > > > > > robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; > > > > > > > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > > festevam@xxxxxxxxx; > > > > > > > vkoul@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > > > > > > > devicetree@xxxxxxxxxxxxxxx; > > > > > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > > > > > > linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; > > > > > > > kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > > > > > > > Oliver Brown <oliver.brown@xxxxxxx>; > > > > > > > alexander.stein@xxxxxxxxxxxxxxx; sam@xxxxxxxxxxxx > > > > > > > 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