| Message ID | 20241206-drm-connector-eld-mutex-v2-4-c9bce1ee8bea@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/connector: add eld_mutex to protect connector->eld | expand |
On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: > Reading access to connector->eld can happen at the same time the > drm_edid_to_eld() updates the data. Take the newly added eld_mutex in > order to protect connector->eld from concurrent access. > > Reviewed-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ > 1 file changed, 2 insertions(+) Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one and to the radeon patches? I'd like to be able to pick the series for drm-misc and these two are not reviewed by you. > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, > continue; > > *enabled = true; > + mutex_lock(&connector->eld_mutex); > ret = drm_eld_size(connector->eld); > memcpy(buf, connector->eld, min(max_bytes, ret)); > + mutex_unlock(&connector->eld_mutex); > > break; > } > > -- > 2.39.5 >
On 2024-12-10 16:20, Dmitry Baryshkov wrote: > On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: >> Reading access to connector->eld can happen at the same time the >> drm_edid_to_eld() updates the data. Take the newly added eld_mutex in >> order to protect connector->eld from concurrent access. >> >> Reviewed-by: Maxime Ripard <mripard@kernel.org> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ >> 1 file changed, 2 insertions(+) > > Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one > and to the radeon patches? I'd like to be able to pick the series for > drm-misc and these two are not reviewed by you. > >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, >> continue; >> >> *enabled = true; >> + mutex_lock(&connector->eld_mutex); >> ret = drm_eld_size(connector->eld); >> memcpy(buf, connector->eld, min(max_bytes, ret)); >> + mutex_unlock(&connector->eld_mutex); All of this is wrapped by the adev->dm.audio_lock mutex. It might be safer to modify the audio_lock mutex so it only guards the aconnector->audio_inst access. But I don't see any way these mutexes would otherwise interact, so this change should be good as-is. Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry >> >> break; >> } >> >> -- >> 2.39.5 >> >
On Mon, 16 Dec 2024 at 16:53, Harry Wentland <harry.wentland@amd.com> wrote: > > > > On 2024-12-10 16:20, Dmitry Baryshkov wrote: > > On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: > >> Reading access to connector->eld can happen at the same time the > >> drm_edid_to_eld() updates the data. Take the newly added eld_mutex in > >> order to protect connector->eld from concurrent access. > >> > >> Reviewed-by: Maxime Ripard <mripard@kernel.org> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> --- > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ > >> 1 file changed, 2 insertions(+) > > > > Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one > > and to the radeon patches? I'd like to be able to pick the series for > > drm-misc and these two are not reviewed by you. > > > >> > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, > >> continue; > >> > >> *enabled = true; > >> + mutex_lock(&connector->eld_mutex); > >> ret = drm_eld_size(connector->eld); > >> memcpy(buf, connector->eld, min(max_bytes, ret)); > >> + mutex_unlock(&connector->eld_mutex); > > All of this is wrapped by the adev->dm.audio_lock mutex. It might > be safer to modify the audio_lock mutex so it only guards the > aconnector->audio_inst access. > > But I don't see any way these mutexes would otherwise interact, > so this change should be good as-is. > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> Would you ack it to merge it through drm-misc? Or would you prefer to pick up those two patches after merging drm-misc-next once the rest of the series lands? > > Harry > > >> > >> break; > >> } > >> > >> -- > >> 2.39.5 > >> > > >
On Mon, Dec 16, 2024 at 10:12 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 16 Dec 2024 at 16:53, Harry Wentland <harry.wentland@amd.com> wrote: > > > > > > > > On 2024-12-10 16:20, Dmitry Baryshkov wrote: > > > On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: > > >> Reading access to connector->eld can happen at the same time the > > >> drm_edid_to_eld() updates the data. Take the newly added eld_mutex in > > >> order to protect connector->eld from concurrent access. > > >> > > >> Reviewed-by: Maxime Ripard <mripard@kernel.org> > > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > >> --- > > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > > > > > Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one > > > and to the radeon patches? I'd like to be able to pick the series for > > > drm-misc and these two are not reviewed by you. > > > > > >> > > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > >> index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 > > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > >> @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, > > >> continue; > > >> > > >> *enabled = true; > > >> + mutex_lock(&connector->eld_mutex); > > >> ret = drm_eld_size(connector->eld); > > >> memcpy(buf, connector->eld, min(max_bytes, ret)); > > >> + mutex_unlock(&connector->eld_mutex); > > > > All of this is wrapped by the adev->dm.audio_lock mutex. It might > > be safer to modify the audio_lock mutex so it only guards the > > aconnector->audio_inst access. > > > > But I don't see any way these mutexes would otherwise interact, > > so this change should be good as-is. > > > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > Would you ack it to merge it through drm-misc? Or would you prefer to > pick up those two patches after merging drm-misc-next once the rest of > the series lands? Merging through drm-misc is fine with me. Thanks, Alex > > > > > Harry > > > > >> > > >> break; > > >> } > > >> > > >> -- > > >> 2.39.5 > > >> > > > > > > > > -- > With best wishes > Dmitry
On Mon, 16 Dec 2024 at 17:32, Alex Deucher <alexdeucher@gmail.com> wrote: > > On Mon, Dec 16, 2024 at 10:12 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 16 Dec 2024 at 16:53, Harry Wentland <harry.wentland@amd.com> wrote: > > > > > > > > > > > > On 2024-12-10 16:20, Dmitry Baryshkov wrote: > > > > On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: > > > >> Reading access to connector->eld can happen at the same time the > > > >> drm_edid_to_eld() updates the data. Take the newly added eld_mutex in > > > >> order to protect connector->eld from concurrent access. > > > >> > > > >> Reviewed-by: Maxime Ripard <mripard@kernel.org> > > > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > >> --- > > > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ > > > >> 1 file changed, 2 insertions(+) > > > > > > > > Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one > > > > and to the radeon patches? I'd like to be able to pick the series for > > > > drm-misc and these two are not reviewed by you. > > > > > > > >> > > > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 > > > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, > > > >> continue; > > > >> > > > >> *enabled = true; > > > >> + mutex_lock(&connector->eld_mutex); > > > >> ret = drm_eld_size(connector->eld); > > > >> memcpy(buf, connector->eld, min(max_bytes, ret)); > > > >> + mutex_unlock(&connector->eld_mutex); > > > > > > All of this is wrapped by the adev->dm.audio_lock mutex. It might > > > be safer to modify the audio_lock mutex so it only guards the > > > aconnector->audio_inst access. > > > > > > But I don't see any way these mutexes would otherwise interact, > > > so this change should be good as-is. > > > > > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > > > Would you ack it to merge it through drm-misc? Or would you prefer to > > pick up those two patches after merging drm-misc-next once the rest of > > the series lands? > > Merging through drm-misc is fine with me. Thanks! > > Thanks, > > Alex > > > > > > > > > Harry > > > > > > >> > > > >> break; > > > >> } > > > >> > > > >> -- > > > >> 2.39.5 > > > >> > > > > > > > > > > > > > -- > > With best wishes > > Dmitry
On 2024-12-16 10:31, Alex Deucher wrote: > On Mon, Dec 16, 2024 at 10:12 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On Mon, 16 Dec 2024 at 16:53, Harry Wentland <harry.wentland@amd.com> wrote: >>> >>> >>> >>> On 2024-12-10 16:20, Dmitry Baryshkov wrote: >>>> On Fri, Dec 06, 2024 at 11:43:07AM +0200, Dmitry Baryshkov wrote: >>>>> Reading access to connector->eld can happen at the same time the >>>>> drm_edid_to_eld() updates the data. Take the newly added eld_mutex in >>>>> order to protect connector->eld from concurrent access. >>>>> >>>>> Reviewed-by: Maxime Ripard <mripard@kernel.org> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>> >>>> Harry, Leo, Rodrigo, Alex, Christian, Xinhui, any response to this one >>>> and to the radeon patches? I'd like to be able to pick the series for >>>> drm-misc and these two are not reviewed by you. >>>> >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 >>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, >>>>> continue; >>>>> >>>>> *enabled = true; >>>>> + mutex_lock(&connector->eld_mutex); >>>>> ret = drm_eld_size(connector->eld); >>>>> memcpy(buf, connector->eld, min(max_bytes, ret)); >>>>> + mutex_unlock(&connector->eld_mutex); >>> >>> All of this is wrapped by the adev->dm.audio_lock mutex. It might >>> be safer to modify the audio_lock mutex so it only guards the >>> aconnector->audio_inst access. >>> >>> But I don't see any way these mutexes would otherwise interact, >>> so this change should be good as-is. >>> >>> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> >> Would you ack it to merge it through drm-misc? Or would you prefer to >> pick up those two patches after merging drm-misc-next once the rest of >> the series lands? > > Merging through drm-misc is fine with me. > Same. Harry > Thanks, > > Alex > >> >>> >>> Harry >>> >>>>> >>>>> break; >>>>> } >>>>> >>>>> -- >>>>> 2.39.5 >>>>> >>>> >>> >> >> >> -- >> With best wishes >> Dmitry
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 19a58630e774029767bf2a27eb4ddf17e3c21129..04c68c320252b5ce9647f0606fb86fe57f347639 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1037,8 +1037,10 @@ static int amdgpu_dm_audio_component_get_eld(struct device *kdev, int port, continue; *enabled = true; + mutex_lock(&connector->eld_mutex); ret = drm_eld_size(connector->eld); memcpy(buf, connector->eld, min(max_bytes, ret)); + mutex_unlock(&connector->eld_mutex); break; }