Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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
>> >> ---
>> >>
>> >> 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(>lock);
>> >> rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
>> >> dp->link_config.max_lanes, dp->config.bpp);
>> >> + mutex_unlock(>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(>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(>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(>lock);
>> >> dp->enabled = false;
>> >> cancel_delayed_work(>hpd_work);
>> >> zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
>>
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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
> >> ---
> >>
> >> 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(>lock);
> >>rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> >> dp->link_config.max_lanes, dp->config.bpp);
> >> + mutex_unlock(>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(>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(>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(>lock);
> >>dp->enabled = false;
> >>cancel_delayed_work(>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(>lock);
> >>
> >>
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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
>> ---
>>
>> 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(>lock);
>> rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
>>dp->link_config.max_lanes, dp->config.bpp);
>> +mutex_unlock(>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(>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(>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(>lock);
>> dp->enabled = false;
>> cancel_delayed_work(>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(>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(>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
>>
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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
> ---
>
> 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(>lock);
> rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> dp->link_config.max_lanes, dp->config.bpp);
> + mutex_unlock(>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(>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(>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(>lock);
> dp->enabled = false;
> cancel_delayed_work(>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(>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(>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(>lock);
> return connector_status_connected;
> }
>
> disconnected:
> dp->status = connector_status_disconnected;
> + mutex_unlock(>lock);
> return connector_status_disconnected;
> }
>
> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct
> *work)
> drm_bridge_hpd_notify(>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];
> +
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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-...@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-...@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
> | Closes:
> https://lore.kernel.org/oe-kbuild-all/202403161747.trmfawhm-...@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 Kwon2018-07-07 275
> d76271d22694e8 Hyun Kwon2018-07-07 276 /**
> d76271d22694e8 Hyun Kwon2018-07-07 277 * struct zynqmp_dp - Xilinx
> DisplayPort core
> d76271d22694e8 Hyun Kwon2018-07-07 278 * @dev: device structure
> d76271d22694e8 Hyun Kwon2018-07-07 279 * @dpsub: Display subsystem
> d76271d22694e8 Hyun Kwon2018-07-07 280 * @iomem: device I/O memory
> for register access
> d76271d22694e8 Hyun Kwon2018-07-07 281 * @reset: reset controller
> 8ce380e6568015 Sean Anderson2024-03-15 282 * @lock: Mutex protecting
> this struct and register access (but not AUX)
> d76271d22694e8 Hyun Kwon2018-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 Kwon2018-07-07 286 * @config: IP core
> configuration from DTS
> d76271d22694e8 Hyun Kwon2018-07-07 287 * @aux: aux channel
> d76271d22694e8 Hyun Kwon2018-07-07 288 * @phy: PHY handles for DP
> lanes
> d76271d22694e8 Hyun Kwon2018-07-07 289 * @num_lanes: number of
> enabled phy lanes
> d76271d22694e8 Hyun Kwon2018-07-07 290 * @hpd_work: hot plug
> detection worker
> d76271d22694e8 Hyun Kwon2018-07-07 291 * @status: connection status
> d76271d22694e8 Hyun Kwon2018-07-07 292 * @enabled: flag to
> indicate if the device is enabled
> d76271d22694e8 Hyun Kwon2018-07-07 293 * @dpcd: DP configuration
> data from currently connected sink device
> d76271d22694e8 Hyun Kwon2018-07-07 294 * @link_config: common link
> configuration between IP core and sink device
> d76271d22694e8 Hyun Kwon2018-07-07 295 * @mode: current mode
> between IP core and sink device
> d76271d22694e8 Hyun Kwon2018-07-07 296 * @train_set: set of
> training data
> d76271d22694e8 Hyun Kwon2018-07-07 297 */
> d76271d22694e8 Hyun Kwon2018-07-07 298 struct zynqmp_dp {
> d76271d22694e8 Hyun Kwon2018-07-07 299 struct device *dev;
> d76271d22694e8 Hyun Kwon2018-07-07 300 struct zynqmp_dpsub
> *dpsub;
> d76271d22694e8 Hyun Kwon2018-07-07 301 void __iomem *iomem;
> d76271d22694e8 Hyun Kwon2018-07-07 302 struct reset_control
> *reset;
> 8ce380e6568015 Sean Anderson2024-03-15 303 struct mutex lock;
> d76271d22694e8 Hyun Kwon2018-07-07 304 int irq;
> d76271d22694e8 Hyun Kwon2018-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 Kwon2018-07-07 309 struct zynqmp_dp_config
> config;
> d76271d22694e8 Hyun Kwon2018-07-07 310 struct drm_dp_aux aux;
> d76271d22694e8 Hyun
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
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-...@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-...@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
| Closes:
https://lore.kernel.org/oe-kbuild-all/202403161747.trmfawhm-...@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 Kwon2018-07-07 275
d76271d22694e8 Hyun Kwon2018-07-07 276 /**
d76271d22694e8 Hyun Kwon2018-07-07 277 * struct zynqmp_dp - Xilinx
DisplayPort core
d76271d22694e8 Hyun Kwon2018-07-07 278 * @dev: device structure
d76271d22694e8 Hyun Kwon2018-07-07 279 * @dpsub: Display subsystem
d76271d22694e8 Hyun Kwon2018-07-07 280 * @iomem: device I/O memory
for register access
d76271d22694e8 Hyun Kwon2018-07-07 281 * @reset: reset controller
8ce380e6568015 Sean Anderson2024-03-15 282 * @lock: Mutex protecting
this struct and register access (but not AUX)
d76271d22694e8 Hyun Kwon2018-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 Kwon2018-07-07 286 * @config: IP core
configuration from DTS
d76271d22694e8 Hyun Kwon2018-07-07 287 * @aux: aux channel
d76271d22694e8 Hyun Kwon2018-07-07 288 * @phy: PHY handles for DP
lanes
d76271d22694e8 Hyun Kwon2018-07-07 289 * @num_lanes: number of
enabled phy lanes
d76271d22694e8 Hyun Kwon2018-07-07 290 * @hpd_work: hot plug
detection worker
d76271d22694e8 Hyun Kwon2018-07-07 291 * @status: connection status
d76271d22694e8 Hyun Kwon2018-07-07 292 * @enabled: flag to indicate
if the device is enabled
d76271d22694e8 Hyun Kwon2018-07-07 293 * @dpcd: DP configuration
data from currently connected sink device
d76271d22694e8 Hyun Kwon2018-07-07 294 * @link_config: common link
configuration between IP core and sink device
d76271d22694e8 Hyun Kwon2018-07-07 295 * @mode: current mode between
IP core and sink device
d76271d22694e8 Hyun Kwon2018-07-07 296 * @train_set: set of training
data
d76271d22694e8 Hyun Kwon2018-07-07 297 */
d76271d22694e8 Hyun Kwon2018-07-07 298 struct zynqmp_dp {
d76271d22694e8 Hyun Kwon2018-07-07 299 struct device *dev;
d76271d22694e8 Hyun Kwon2018-07-07 300 struct zynqmp_dpsub
*dpsub;
d76271d22694e8 Hyun Kwon2018-07-07 301 void __iomem *iomem;
d76271d22694e8 Hyun Kwon2018-07-07 302 struct reset_control
*reset;
8ce380e6568015 Sean Anderson2024-03-15 303 struct mutex lock;
d76271d22694e8 Hyun Kwon2018-07-07 304 int irq;
d76271d22694e8 Hyun Kwon2018-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 Kwon2018-07-07 309 struct zynqmp_dp_config
config;
d76271d22694e8 Hyun Kwon2018-07-07 310 struct drm_dp_aux aux;
d76271d22694e8 Hyun Kwon2018-07-07 311 struct phy
*phy[ZYNQMP_DP_MAX_LANES];
d76271d22694e8 Hyun Kwon2018-07-07 312 u8 num_lanes;
8ce380e6568015 Sean Anderson2024-03-15 313 struct delayed_work
hpd_work, hpd_irq_work;
d76271d22694e8 Hyun Kwon2018-07-07 314 enum
drm_connector_status status;
d76271d22694e8 Hyun Kwon2018-07-07 315 bool enabled;
d76271d22694e8 Hyun Kwon2018-07-07 316
d76271d22694e8 Hyun Kwon2018-07-07 31
[PATCH 3/6] drm: zynqmp_dp: Add locking
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
---
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)
* @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(>lock);
rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
dp->link_config.max_lanes, dp->config.bpp);
+ mutex_unlock(>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(>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(>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(>lock);
dp->enabled = false;
cancel_delayed_work(>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(>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(>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(>lock);
return connector_status_connected;
}
disconnected:
dp->status = connector_status_disconnected;
+ mutex_unlock(>lock);
return connector_status_disconnected;
}
@@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct
*work)
drm_bridge_hpd_notify(>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(>lock);
+ err = drm_dp_dpcd_read(>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([2], dp->mode.lane_cnt) ||
+ !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
+ zynqmp_dp_train_loop(dp);
+ }
+ }
+ mutex_unlock(>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
