Hi Sean,
On Mon, Mar 18, 2024 at 01:29:12PM -0400, Sean Anderson wrote:
> On 3/18/24 13:16, Laurent Pinchart wrote:
> > On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
> >> Add some locking, since none is provided by the drm subsystem. This will
> >
> > That's not quite right, the DRM core doesn't call bridge operations
> > concurrently.
>
> I figured something like this was going on.
>
> > We may need locking to protect against race conditions
> > between bridge operations and interrupts though.
>
> And of course this will only get worse once we let userspace get involved.
>
> >> prevent the IRQ/workers/bridge API calls from stepping on each other's
> >> toes.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> >> ---
> >>
> >> drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
> >> 1 file changed, 42 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> index 8635b5673386..d2dee58e7bf2 100644
> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
> >> * @dpsub: Display subsystem
> >> * @iomem: device I/O memory for register access
> >> * @reset: reset controller
> >> + * @lock: Mutex protecting this struct and register access (but not AUX)
> >
> > This patch does two things at once, it defers link training from the IRQ
> > handler to a work queue, and covers everything with a big lock. The
> > scope is too large.
>
> OK, I can split this.
>
> > Please restrict the lock scope and document the
> > individual fields that need to be protected, and explain the locking
> > design in the commit message (or comments in the code).
>
> As said, this lock protects
>
> - Non-atomic registers configuring the link. That is, everything but the IRQ
> registers (since these are accessed in an atomic fashion), and the DP AUX
> registers (since these don't affect the link).
> - Link configuration. This is effectively everything in zynqmp_dp which isn't
> read-only after probe time. So from next_bridge onward.
>
> It's designed to protect configuration changes so we don't have to do anything
> tricky. Configuration should never be in the hot path, so I'm not worried about
> performance.
If userspace can control all this directly through debugfs, can you
guarantee that locks will be enough ? The driver doesn't expect direct
userspace access. I have a feeling this is really quite hacky.
> >> * @irq: irq
> >> * @bridge: DRM bridge for the DP encoder
> >> * @next_bridge: The downstream bridge
> >> @@ -299,6 +300,7 @@ struct zynqmp_dp {
> >> struct zynqmp_dpsub *dpsub;
> >> void __iomem *iomem;
> >> struct reset_control *reset;
> >> + struct mutex lock;
> >> int irq;
> >>
> >> struct drm_bridge bridge;
> >> @@ -308,7 +310,7 @@ struct zynqmp_dp {
> >> struct drm_dp_aux aux;
> >> struct phy *phy[ZYNQMP_DP_MAX_LANES];
> >> u8 num_lanes;
> >> - struct delayed_work hpd_work;
> >> + struct delayed_work hpd_work, hpd_irq_work;
> >
> > One variable per line please.
>
> OK
>
> >> enum drm_connector_status status;
> >> bool enabled;
> >>
> >> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
> >> }
> >>
> >> /* Check with link rate and lane count */
> >> + mutex_lock(&dp->lock);
> >> rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> >> dp->link_config.max_lanes, dp->config.bpp);
> >> + mutex_unlock(&dp->lock);
> >> if (mode->clock > rate) {
> >> dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
> >> mode->name);
> >> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
> >>
> >> pm_runtime_get_sync(dp->dev);
> >>
> >> + mutex_lock(&dp->lock);
> >> zynqmp_dp_disp_enable(dp, old_bridge_state);
> >>
> >> /*
> >> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
> >> zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
> >> ZYNQMP_DP_SOFTWARE_RESET_ALL);
> >> zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
> >> + mutex_unlock(&dp->lock);
> >> }
> >>
> >> static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >> @@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >> {
> >> struct zynqmp_dp *dp = bridge_to_dp(bridge);
> >>
> >> + mutex_lock(&dp->lock);
> >> dp->enabled = false;
> >> cancel_delayed_work(&dp->hpd_work);
> >> zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
> >> @@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >> zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
> >>
> >> zynqmp_dp_disp_disable(dp, old_bridge_state);
> >> + mutex_unlock(&dp->lock);
> >>
> >> pm_runtime_put_sync(dp->dev);
> >> }
> >> @@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
> >> u32 state, i;
> >> int ret;
> >>
> >> + mutex_lock(&dp->lock);
> >> +
> >> /*
> >> * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
> >> * get the HPD signal with some monitors.
> >> @@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
> >> dp->num_lanes);
> >>
> >> dp->status = connector_status_connected;
> >> + mutex_unlock(&dp->lock);
> >> return connector_status_connected;
> >> }
> >>
> >> disconnected:
> >> dp->status = connector_status_disconnected;
> >> + mutex_unlock(&dp->lock);
> >> return connector_status_disconnected;
> >> }
> >>
> >> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
> >> drm_bridge_hpd_notify(&dp->bridge, status);
> >> }
> >>
> >> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> >> +{
> >> + struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
> >> + hpd_irq_work.work);
> >> + u8 status[DP_LINK_STATUS_SIZE + 2];
> >> + int err;
> >> +
> >> + mutex_lock(&dp->lock);
> >> + err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> >> + DP_LINK_STATUS_SIZE + 2);
> >> + if (err < 0) {
> >> + dev_dbg_ratelimited(dp->dev,
> >> + "could not read sink status: %d\n", err);
> >> + } else {
> >> + if (status[4] & DP_LINK_STATUS_UPDATED ||
> >> + !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> >> + !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> >> + zynqmp_dp_train_loop(dp);
> >> + }
> >> + }
> >> + mutex_unlock(&dp->lock);
> >> +}
> >> +
> >> static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> >> {
> >> struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
> >> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> >> if (status & ZYNQMP_DP_INT_HPD_EVENT)
> >> schedule_delayed_work(&dp->hpd_work, 0);
> >>
> >> - if (status & ZYNQMP_DP_INT_HPD_IRQ) {
> >> - int ret;
> >> - u8 status[DP_LINK_STATUS_SIZE + 2];
> >> + if (status & ZYNQMP_DP_INT_HPD_IRQ)
> >> + schedule_delayed_work(&dp->hpd_irq_work, 0);
> >>
> >> - ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> >> - DP_LINK_STATUS_SIZE + 2);
> >> - if (ret < 0)
> >> - goto handled;
> >> -
> >> - if (status[4] & DP_LINK_STATUS_UPDATED ||
> >> - !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> >> - !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> >> - zynqmp_dp_train_loop(dp);
> >> - }
> >> - }
> >> -
> >> -handled:
> >> return IRQ_HANDLED;
> >> }
> >>
> >> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> >> dp->dev = &pdev->dev;
> >> dp->dpsub = dpsub;
> >> dp->status = connector_status_disconnected;
> >> + mutex_init(&dp->lock);
> >>
> >> INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
> >> + INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
> >>
> >> /* Acquire all resources (IOMEM, IRQ and PHYs). */
> >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> >> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> >> zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
> >> disable_irq(dp->irq);
> >>
> >> + cancel_delayed_work_sync(&dp->hpd_irq_work);
> >> cancel_delayed_work_sync(&dp->hpd_work);
> >>
> >> zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
> >> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> >>
> >> zynqmp_dp_phy_exit(dp);
> >> zynqmp_dp_reset(dp, true);
> >> + mutex_destroy(&dp->lock);
> >> }
--
Regards,
Laurent Pinchart
