Intrenal_hpd is referenced by event thread but set by drm bridge callback
context. Add mutex to protect internal_hpd to avoid conflicts between
threads.
Signed-off-by: Kuogee Hsieh <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 71aa944..b59ea7a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1792,11 +1792,13 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
dp = container_of(dp_display, struct dp_display_private, dp_display);
+ mutex_lock(&dp->event_mutex);
dp_display->internal_hpd = true;
dp_catalog_hpd_config_intr(dp->catalog,
DP_DP_HPD_PLUG_INT_MASK |
DP_DP_HPD_UNPLUG_INT_MASK,
true);
+ mutex_unlock(&dp->event_mutex);
}
void dp_bridge_hpd_disable(struct drm_bridge *bridge)
@@ -1807,11 +1809,13 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
dp = container_of(dp_display, struct dp_display_private, dp_display);
+ mutex_lock(&dp->event_mutex);
dp_catalog_hpd_config_intr(dp->catalog,
DP_DP_HPD_PLUG_INT_MASK |
DP_DP_HPD_UNPLUG_INT_MASK,
false);
dp_display->internal_hpd = false;
+ mutex_unlock(&dp->event_mutex);
}
void dp_bridge_hpd_notify(struct drm_bridge *bridge,
@@ -1822,8 +1826,12 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
/* Without next_bridge interrupts are handled by the DP core directly */
- if (dp_display->internal_hpd)
+ mutex_lock(&dp->event_mutex);
+ if (dp_display->internal_hpd) {
+ mutex_unlock(&dp->event_mutex);
return;
+ }
+ mutex_unlock(&dp->event_mutex);
if (!dp->core_initialized) {
drm_dbg_dp(dp->drm_dev, "not initialized\n");
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Quoting Kuogee Hsieh (2023-05-10 13:31:05)
> Intrenal_hpd is referenced by event thread but set by drm bridge callback
> context. Add mutex to protect internal_hpd to avoid conflicts between
> threads.
>
> Signed-off-by: Kuogee Hsieh <[email protected]>
> ---
This patch looks completely unnecessary. How can dp_bridge_hpd_enable()
be called at the same time that dp_bridge_hpd_disable() is called or
dp_bridge_hpd_notify() is called? Isn't there locking or ordering at a
higher layer?
On 5/10/2023 3:46 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-05-10 13:31:05)
>> Intrenal_hpd is referenced by event thread but set by drm bridge callback
>> context. Add mutex to protect internal_hpd to avoid conflicts between
>> threads.
>>
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> ---
>
> This patch looks completely unnecessary. How can dp_bridge_hpd_enable()
> be called at the same time that dp_bridge_hpd_disable() is called or
> dp_bridge_hpd_notify() is called? Isn't there locking or ordering at a
> higher layer?
Ack. We can drop this patch because we are protected by
bridge->hpd_mutex in drm_bridge_hpd_enable() / drm_bridge_hpd_disable ()
and drm_bridge_hpd_notify().
internal_hpd is referenced at both plug and unplug handle.
The majority purpose of mutext is try to serialize internal_hpd between
dp_bridge_hpd_disable() and either plug or unplug handle.
On 5/10/2023 4:11 PM, Abhinav Kumar wrote:
>
>
> On 5/10/2023 3:46 PM, Stephen Boyd wrote:
>> Quoting Kuogee Hsieh (2023-05-10 13:31:05)
>>> Intrenal_hpd is referenced by event thread but set by drm bridge
>>> callback
>>> context. Add mutex to protect internal_hpd to avoid conflicts between
>>> threads.
>>>
>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>> ---
>>
>> This patch looks completely unnecessary. How can dp_bridge_hpd_enable()
>> be called at the same time that dp_bridge_hpd_disable() is called or
>> dp_bridge_hpd_notify() is called? Isn't there locking or ordering at a
>> higher layer?
>
> Ack. We can drop this patch because we are protected by
> bridge->hpd_mutex in drm_bridge_hpd_enable() / drm_bridge_hpd_disable
> () and drm_bridge_hpd_notify().
Hi Stephen
On 5/10/2023 4:19 PM, Kuogee Hsieh wrote:
> internal_hpd is referenced at both plug and unplug handle.
>
> The majority purpose of mutext is try to serialize internal_hpd between
> dp_bridge_hpd_disable() and either plug or unplug handle.
>
>
> On 5/10/2023 4:11 PM, Abhinav Kumar wrote:
>>
>>
>> On 5/10/2023 3:46 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2023-05-10 13:31:05)
>>>> Intrenal_hpd is referenced by event thread but set by drm bridge
>>>> callback
>>>> context. Add mutex to protect internal_hpd to avoid conflicts between
>>>> threads.
>>>>
>>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>>> ---
>>>
>>> This patch looks completely unnecessary. How can dp_bridge_hpd_enable()
>>> be called at the same time that dp_bridge_hpd_disable() is called or
>>> dp_bridge_hpd_notify() is called? Isn't there locking or ordering at a
>>> higher layer?
>>
>> Ack. We can drop this patch because we are protected by
>> bridge->hpd_mutex in drm_bridge_hpd_enable() / drm_bridge_hpd_disable
>> () and drm_bridge_hpd_notify().
I understood now, so what kuogee is referring to is that this
event_mutex protection is to not protect those 3 calls from each other
(since they are already protected as we saw above) but because
dp_hpd_plug_handle/dp_hpd_unplug_handle still uses
dp_display.internal_hpd to re-enable the hot-plug interrupt, this is
making sure that flow is protected as well.