| Message ID | 20240315230916.1759060-4-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: zynqmp_dp: Misc. patches and debugfs support | expand |
Hi Sean,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.8]
[also build test WARNING on linus/master next-20240315]
[cannot apply to drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
base: v6.8
patch link: https://lore.kernel.org/r/20240315230916.1759060-4-sean.anderson%40linux.dev
patch subject: [PATCH 3/6] drm: zynqmp_dp: Add locking
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403161747.TRmfawhM-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/xlnx/zynqmp_dp.c:321: warning: Function parameter or struct member 'hpd_irq_work' not described in 'zynqmp_dp'
vim +321 drivers/gpu/drm/xlnx/zynqmp_dp.c
d76271d22694e8 Hyun Kwon 2018-07-07 275
d76271d22694e8 Hyun Kwon 2018-07-07 276 /**
d76271d22694e8 Hyun Kwon 2018-07-07 277 * struct zynqmp_dp - Xilinx DisplayPort core
d76271d22694e8 Hyun Kwon 2018-07-07 278 * @dev: device structure
d76271d22694e8 Hyun Kwon 2018-07-07 279 * @dpsub: Display subsystem
d76271d22694e8 Hyun Kwon 2018-07-07 280 * @iomem: device I/O memory for register access
d76271d22694e8 Hyun Kwon 2018-07-07 281 * @reset: reset controller
8ce380e6568015 Sean Anderson 2024-03-15 282 * @lock: Mutex protecting this struct and register access (but not AUX)
d76271d22694e8 Hyun Kwon 2018-07-07 283 * @irq: irq
47e801bd0749f0 Laurent Pinchart 2021-08-04 284 * @bridge: DRM bridge for the DP encoder
bd68b9b3cb2e0d Laurent Pinchart 2021-08-04 285 * @next_bridge: The downstream bridge
d76271d22694e8 Hyun Kwon 2018-07-07 286 * @config: IP core configuration from DTS
d76271d22694e8 Hyun Kwon 2018-07-07 287 * @aux: aux channel
d76271d22694e8 Hyun Kwon 2018-07-07 288 * @phy: PHY handles for DP lanes
d76271d22694e8 Hyun Kwon 2018-07-07 289 * @num_lanes: number of enabled phy lanes
d76271d22694e8 Hyun Kwon 2018-07-07 290 * @hpd_work: hot plug detection worker
d76271d22694e8 Hyun Kwon 2018-07-07 291 * @status: connection status
d76271d22694e8 Hyun Kwon 2018-07-07 292 * @enabled: flag to indicate if the device is enabled
d76271d22694e8 Hyun Kwon 2018-07-07 293 * @dpcd: DP configuration data from currently connected sink device
d76271d22694e8 Hyun Kwon 2018-07-07 294 * @link_config: common link configuration between IP core and sink device
d76271d22694e8 Hyun Kwon 2018-07-07 295 * @mode: current mode between IP core and sink device
d76271d22694e8 Hyun Kwon 2018-07-07 296 * @train_set: set of training data
d76271d22694e8 Hyun Kwon 2018-07-07 297 */
d76271d22694e8 Hyun Kwon 2018-07-07 298 struct zynqmp_dp {
d76271d22694e8 Hyun Kwon 2018-07-07 299 struct device *dev;
d76271d22694e8 Hyun Kwon 2018-07-07 300 struct zynqmp_dpsub *dpsub;
d76271d22694e8 Hyun Kwon 2018-07-07 301 void __iomem *iomem;
d76271d22694e8 Hyun Kwon 2018-07-07 302 struct reset_control *reset;
8ce380e6568015 Sean Anderson 2024-03-15 303 struct mutex lock;
d76271d22694e8 Hyun Kwon 2018-07-07 304 int irq;
d76271d22694e8 Hyun Kwon 2018-07-07 305
47e801bd0749f0 Laurent Pinchart 2021-08-04 306 struct drm_bridge bridge;
bd68b9b3cb2e0d Laurent Pinchart 2021-08-04 307 struct drm_bridge *next_bridge;
47e801bd0749f0 Laurent Pinchart 2021-08-04 308
d76271d22694e8 Hyun Kwon 2018-07-07 309 struct zynqmp_dp_config config;
d76271d22694e8 Hyun Kwon 2018-07-07 310 struct drm_dp_aux aux;
d76271d22694e8 Hyun Kwon 2018-07-07 311 struct phy *phy[ZYNQMP_DP_MAX_LANES];
d76271d22694e8 Hyun Kwon 2018-07-07 312 u8 num_lanes;
8ce380e6568015 Sean Anderson 2024-03-15 313 struct delayed_work hpd_work, hpd_irq_work;
d76271d22694e8 Hyun Kwon 2018-07-07 314 enum drm_connector_status status;
d76271d22694e8 Hyun Kwon 2018-07-07 315 bool enabled;
d76271d22694e8 Hyun Kwon 2018-07-07 316
d76271d22694e8 Hyun Kwon 2018-07-07 317 u8 dpcd[DP_RECEIVER_CAP_SIZE];
d76271d22694e8 Hyun Kwon 2018-07-07 318 struct zynqmp_dp_link_config link_config;
d76271d22694e8 Hyun Kwon 2018-07-07 319 struct zynqmp_dp_mode mode;
d76271d22694e8 Hyun Kwon 2018-07-07 320 u8 train_set[ZYNQMP_DP_MAX_LANES];
d76271d22694e8 Hyun Kwon 2018-07-07 @321 };
d76271d22694e8 Hyun Kwon 2018-07-07 322
On 3/16/24 05:52, kernel test robot wrote: > Hi Sean, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on v6.8] > [also build test WARNING on linus/master next-20240315] > [cannot apply to drm-misc/drm-misc-next] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208 > base: v6.8 > patch link: https://lore.kernel.org/r/20240315230916.1759060-4-sean.anderson%40linux.dev > patch subject: [PATCH 3/6] drm: zynqmp_dp: Add locking > config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/config) > compiler: alpha-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403161747.TRmfawhM-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:321: warning: Function parameter or struct member 'hpd_irq_work' not described in 'zynqmp_dp' Will document. --Sean > > vim +321 drivers/gpu/drm/xlnx/zynqmp_dp.c > > d76271d22694e8 Hyun Kwon 2018-07-07 275 > d76271d22694e8 Hyun Kwon 2018-07-07 276 /** > d76271d22694e8 Hyun Kwon 2018-07-07 277 * struct zynqmp_dp - Xilinx DisplayPort core > d76271d22694e8 Hyun Kwon 2018-07-07 278 * @dev: device structure > d76271d22694e8 Hyun Kwon 2018-07-07 279 * @dpsub: Display subsystem > d76271d22694e8 Hyun Kwon 2018-07-07 280 * @iomem: device I/O memory for register access > d76271d22694e8 Hyun Kwon 2018-07-07 281 * @reset: reset controller > 8ce380e6568015 Sean Anderson 2024-03-15 282 * @lock: Mutex protecting this struct and register access (but not AUX) > d76271d22694e8 Hyun Kwon 2018-07-07 283 * @irq: irq > 47e801bd0749f0 Laurent Pinchart 2021-08-04 284 * @bridge: DRM bridge for the DP encoder > bd68b9b3cb2e0d Laurent Pinchart 2021-08-04 285 * @next_bridge: The downstream bridge > d76271d22694e8 Hyun Kwon 2018-07-07 286 * @config: IP core configuration from DTS > d76271d22694e8 Hyun Kwon 2018-07-07 287 * @aux: aux channel > d76271d22694e8 Hyun Kwon 2018-07-07 288 * @phy: PHY handles for DP lanes > d76271d22694e8 Hyun Kwon 2018-07-07 289 * @num_lanes: number of enabled phy lanes > d76271d22694e8 Hyun Kwon 2018-07-07 290 * @hpd_work: hot plug detection worker > d76271d22694e8 Hyun Kwon 2018-07-07 291 * @status: connection status > d76271d22694e8 Hyun Kwon 2018-07-07 292 * @enabled: flag to indicate if the device is enabled > d76271d22694e8 Hyun Kwon 2018-07-07 293 * @dpcd: DP configuration data from currently connected sink device > d76271d22694e8 Hyun Kwon 2018-07-07 294 * @link_config: common link configuration between IP core and sink device > d76271d22694e8 Hyun Kwon 2018-07-07 295 * @mode: current mode between IP core and sink device > d76271d22694e8 Hyun Kwon 2018-07-07 296 * @train_set: set of training data > d76271d22694e8 Hyun Kwon 2018-07-07 297 */ > d76271d22694e8 Hyun Kwon 2018-07-07 298 struct zynqmp_dp { > d76271d22694e8 Hyun Kwon 2018-07-07 299 struct device *dev; > d76271d22694e8 Hyun Kwon 2018-07-07 300 struct zynqmp_dpsub *dpsub; > d76271d22694e8 Hyun Kwon 2018-07-07 301 void __iomem *iomem; > d76271d22694e8 Hyun Kwon 2018-07-07 302 struct reset_control *reset; > 8ce380e6568015 Sean Anderson 2024-03-15 303 struct mutex lock; > d76271d22694e8 Hyun Kwon 2018-07-07 304 int irq; > d76271d22694e8 Hyun Kwon 2018-07-07 305 > 47e801bd0749f0 Laurent Pinchart 2021-08-04 306 struct drm_bridge bridge; > bd68b9b3cb2e0d Laurent Pinchart 2021-08-04 307 struct drm_bridge *next_bridge; > 47e801bd0749f0 Laurent Pinchart 2021-08-04 308 > d76271d22694e8 Hyun Kwon 2018-07-07 309 struct zynqmp_dp_config config; > d76271d22694e8 Hyun Kwon 2018-07-07 310 struct drm_dp_aux aux; > d76271d22694e8 Hyun Kwon 2018-07-07 311 struct phy *phy[ZYNQMP_DP_MAX_LANES]; > d76271d22694e8 Hyun Kwon 2018-07-07 312 u8 num_lanes; > 8ce380e6568015 Sean Anderson 2024-03-15 313 struct delayed_work hpd_work, hpd_irq_work; > d76271d22694e8 Hyun Kwon 2018-07-07 314 enum drm_connector_status status; > d76271d22694e8 Hyun Kwon 2018-07-07 315 bool enabled; > d76271d22694e8 Hyun Kwon 2018-07-07 316 > d76271d22694e8 Hyun Kwon 2018-07-07 317 u8 dpcd[DP_RECEIVER_CAP_SIZE]; > d76271d22694e8 Hyun Kwon 2018-07-07 318 struct zynqmp_dp_link_config link_config; > d76271d22694e8 Hyun Kwon 2018-07-07 319 struct zynqmp_dp_mode mode; > d76271d22694e8 Hyun Kwon 2018-07-07 320 u8 train_set[ZYNQMP_DP_MAX_LANES]; > d76271d22694e8 Hyun Kwon 2018-07-07 @321 }; > d76271d22694e8 Hyun Kwon 2018-07-07 322 >
Hi Sean, Thank you for the patch. 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. We may need locking to protect against race conditions between bridge operations and interrupts though. > prevent the IRQ/workers/bridge API calls from stepping on each other's > toes. > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > 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. 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). > * @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. > 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); > }
On 3/18/24 13:16, Laurent Pinchart wrote: > Hi Sean, > > Thank you for the patch. > > 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@linux.dev> >> --- >> >> 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. >> * @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); >> } >
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@linux.dev> > >> --- > >> > >> 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); > >> }
On 3/18/24 13:59, Laurent Pinchart wrote: > 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@linux.dev> >> >> --- >> >> >> >> 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. Yes, this is fine. The most userspace can do is force a lot of retraining. But we have timeouts on everything so I'm not really concerned. --Sean >> >> * @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); >> >> } >
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) * @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; 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); }
Add some locking, since none is provided by the drm subsystem. This will prevent the IRQ/workers/bridge API calls from stepping on each other's toes. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 17 deletions(-)