| Message ID | 20210924064324.229457-1-greenfoo@u92.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible | expand |
On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote: > Hi all, > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this > patch series is about. > > You will find two types of changes here: > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has > already been done in previous commits such as b7ea04d2) > > - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" > in the remaining places (as it has already been done in previous commits > such as 57037094) > > Most of the changes are straight forward, except for a few cases in the "amd" > and "i915" drivers where some extra dancing was needed to overcome the > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used > once inside the same function (the reason being that the macro expansion > includes *labels*, and you can not have two labels named the same inside one > function) > > Notice that, even after this patch series, some places remain where > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, > all inside drm core (which makes sense), except for two (in "amd" and "i915") > which cannot be replaced due to the way they are being used. > > Changes in v2: > > - Fix commit message typo > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible > - Split drm/i915 patch into two simpler ones > - Remove drm_modeset_(un)lock_all() > - Fix build problems in non-x86 platforms > > Fernando Ramos (17): > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 > drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > drm: cleanup: remove drm_modeset_(un)lock_all() > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along with the necessary drm-tip conflict resolutions). Sean > Documentation/gpu/todo.rst | 17 ---- > Documentation/locking/ww-mutex-design.rst | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 21 +++-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++----- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 25 ++--- > drivers/gpu/drm/drm_client_modeset.c | 14 ++- > drivers/gpu/drm/drm_crtc_helper.c | 18 ++-- > drivers/gpu/drm/drm_fb_helper.c | 10 +- > drivers/gpu/drm/drm_framebuffer.c | 6 +- > drivers/gpu/drm/drm_modeset_lock.c | 94 +------------------ > drivers/gpu/drm/gma500/psb_device.c | 18 ++-- > drivers/gpu/drm/i915/display/intel_audio.c | 16 ++-- > drivers/gpu/drm/i915/display/intel_display.c | 23 ++--- > .../drm/i915/display/intel_display_debugfs.c | 46 +++++---- > drivers/gpu/drm/i915/display/intel_overlay.c | 46 ++++----- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 7 +- > drivers/gpu/drm/i915/i915_drv.c | 13 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 10 +- > .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 12 +-- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 15 ++- > drivers/gpu/drm/omapdrm/omap_fb.c | 9 +- > drivers/gpu/drm/radeon/radeon_device.c | 21 +++-- > drivers/gpu/drm/radeon/radeon_dp_mst.c | 10 +- > drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 +- > drivers/gpu/drm/tegra/dsi.c | 6 +- > drivers/gpu/drm/tegra/hdmi.c | 6 +- > drivers/gpu/drm/tegra/sor.c | 11 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++- > include/drm/drm_modeset_lock.h | 2 - > 30 files changed, 265 insertions(+), 292 deletions(-) > > > base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f > -- > 2.33.0 >
On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote: > > Hi all, > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this > > patch series is about. > > > > You will find two types of changes here: > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has > > already been done in previous commits such as b7ea04d2) > > > > - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" > > in the remaining places (as it has already been done in previous commits > > such as 57037094) > > > > Most of the changes are straight forward, except for a few cases in the "amd" > > and "i915" drivers where some extra dancing was needed to overcome the > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used > > once inside the same function (the reason being that the macro expansion > > includes *labels*, and you can not have two labels named the same inside one > > function) > > > > Notice that, even after this patch series, some places remain where > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, > > all inside drm core (which makes sense), except for two (in "amd" and "i915") > > which cannot be replaced due to the way they are being used. > > > > Changes in v2: > > > > - Fix commit message typo > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible > > - Split drm/i915 patch into two simpler ones > > - Remove drm_modeset_(un)lock_all() > > - Fix build problems in non-x86 platforms > > > > Fernando Ramos (17): > > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 > > drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > drm: cleanup: remove drm_modeset_(un)lock_all() > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > with the necessary drm-tip conflict resolutions). Ugh. Did anyone actually review the locking changes this does? I shot the previous i915 stuff down because the commit messages did not address any of it.
On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote: > > > Hi all, > > > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this > > > patch series is about. > > > > > > You will find two types of changes here: > > > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has > > > already been done in previous commits such as b7ea04d2) > > > > > > - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" > > > in the remaining places (as it has already been done in previous commits > > > such as 57037094) > > > > > > Most of the changes are straight forward, except for a few cases in the "amd" > > > and "i915" drivers where some extra dancing was needed to overcome the > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used > > > once inside the same function (the reason being that the macro expansion > > > includes *labels*, and you can not have two labels named the same inside one > > > function) > > > > > > Notice that, even after this patch series, some places remain where > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, > > > all inside drm core (which makes sense), except for two (in "amd" and "i915") > > > which cannot be replaced due to the way they are being used. > > > > > > Changes in v2: > > > > > > - Fix commit message typo > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible > > > - Split drm/i915 patch into two simpler ones > > > - Remove drm_modeset_(un)lock_all() > > > - Fix build problems in non-x86 platforms > > > > > > Fernando Ramos (17): > > > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 > > > drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > drm: cleanup: remove drm_modeset_(un)lock_all() > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > with the necessary drm-tip conflict resolutions). > > Ugh. Did anyone actually review the locking changes this does? > I shot the previous i915 stuff down because the commit messages > did not address any of it. I reviewed the set on 9/17, I didn't see your feedback on that thread. Sean > > -- > Ville Syrjälä > Intel
On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote: > > > > Hi all, > > > > > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to > > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this > > > > patch series is about. > > > > > > > > You will find two types of changes here: > > > > > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has > > > > already been done in previous commits such as b7ea04d2) > > > > > > > > - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" > > > > in the remaining places (as it has already been done in previous commits > > > > such as 57037094) > > > > > > > > Most of the changes are straight forward, except for a few cases in the "amd" > > > > and "i915" drivers where some extra dancing was needed to overcome the > > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used > > > > once inside the same function (the reason being that the macro expansion > > > > includes *labels*, and you can not have two labels named the same inside one > > > > function) > > > > > > > > Notice that, even after this patch series, some places remain where > > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, > > > > all inside drm core (which makes sense), except for two (in "amd" and "i915") > > > > which cannot be replaced due to the way they are being used. > > > > > > > > Changes in v2: > > > > > > > > - Fix commit message typo > > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible > > > > - Split drm/i915 patch into two simpler ones > > > > - Remove drm_modeset_(un)lock_all() > > > > - Fix build problems in non-x86 platforms > > > > > > > > Fernando Ramos (17): > > > > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 > > > > drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > drm: cleanup: remove drm_modeset_(un)lock_all() > > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > > with the necessary drm-tip conflict resolutions). > > > > Ugh. Did anyone actually review the locking changes this does? > > I shot the previous i915 stuff down because the commit messages > > did not address any of it. > > I reviewed the set on 9/17, I didn't see your feedback on that thread. It was much earlir than that. https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html And I think I might have also shot down a similar thing earlier. I was actually half considering sending a patch to nuke that misleading TODO item. I don't think anything which changes which locks are taken should be considred a starter level task. And the commit messages here don't seem to address any of it.
On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote: > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote: > > > > > Hi all, > > > > > > > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to > > > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this > > > > > patch series is about. > > > > > > > > > > You will find two types of changes here: > > > > > > > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with > > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has > > > > > already been done in previous commits such as b7ea04d2) > > > > > > > > > > - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" > > > > > in the remaining places (as it has already been done in previous commits > > > > > such as 57037094) > > > > > > > > > > Most of the changes are straight forward, except for a few cases in the "amd" > > > > > and "i915" drivers where some extra dancing was needed to overcome the > > > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used > > > > > once inside the same function (the reason being that the macro expansion > > > > > includes *labels*, and you can not have two labels named the same inside one > > > > > function) > > > > > > > > > > Notice that, even after this patch series, some places remain where > > > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, > > > > > all inside drm core (which makes sense), except for two (in "amd" and "i915") > > > > > which cannot be replaced due to the way they are being used. > > > > > > > > > > Changes in v2: > > > > > > > > > > - Fix commit message typo > > > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible > > > > > - Split drm/i915 patch into two simpler ones > > > > > - Remove drm_modeset_(un)lock_all() > > > > > - Fix build problems in non-x86 platforms > > > > > > > > > > Fernando Ramos (17): > > > > > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 > > > > > drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() > > > > > drm: cleanup: remove drm_modeset_(un)lock_all() > > > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup > > > > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > > > with the necessary drm-tip conflict resolutions). > > > > > > Ugh. Did anyone actually review the locking changes this does? > > > I shot the previous i915 stuff down because the commit messages > > > did not address any of it. > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread. > > It was much earlir than that. > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html > > And I think I might have also shot down a similar thing earlier. > > I was actually half considering sending a patch to nuke that > misleading TODO item. I don't think anything which changes > which locks are taken should be considred a starter level task. > And the commit messages here don't seem to address any of it. And i915 is now broken :( https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10680/fi-bwr-2160/boot.html
On 21/10/02 05:30AM, Ville Syrjälä wrote: > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote: > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > > > > with the necessary drm-tip conflict resolutions). > > > > > > > > Ugh. Did anyone actually review the locking changes this does? > > > > I shot the previous i915 stuff down because the commit messages > > > > did not address any of it. > > > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread. > > > > It was much earlir than that. > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html > > > > And I think I might have also shot down a similar thing earlier. > > > > I was actually half considering sending a patch to nuke that > > misleading TODO item. I don't think anything which changes > > which locks are taken should be considred a starter level task. > > And the commit messages here don't seem to address any of it. > > And i915 is now broken :( > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10680/fi-bwr-2160/boot.html I completely overlooked the side effects of not having a global context anymore. Sorry for all the trouble. Sean, could you revert the whole patch series? I'll have a deeper look into the patch set and come up with a v3 where all these issues will be addressed. Thanks and sorry once again for the extra overhead this might have caused.
On 21/10/02 09:13AM, Fernando Ramos wrote: > On 21/10/02 05:30AM, Ville Syrjälä wrote: > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote: > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > > > > > with the necessary drm-tip conflict resolutions). > > > > > > > > > > Ugh. Did anyone actually review the locking changes this does? > > > > > I shot the previous i915 stuff down because the commit messages > > > > > did not address any of it. > > > > > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread. > > > > > > It was much earlir than that. > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html Sorry, I'm new to this and it did not occur to me to search for similar patches in the mailing list archives in case there were additional comments that applied to my change set. In case I had done that I would have found that, as you mentioned, you had already raised two issues back in June: On Tue, Jun 29, 2021, Ville Syrjälä wrote: > > That looks wrong. You're using a private ctx here, but still > passing dev->mode_config.acquire_ctx to the lower level stuff. > > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > mode_config.mutex. So would need a proper review whether we > actually need that lock or not. The first one was pointing out the same error I would later repeat in my patch series (ups). After further inspection of the code it looks to me that changing this: intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); ...into this: intel_modeset_setup_hw_state(dev, &ctx); ...would be enough. Why? The only difference between the old drm_modeset_{lock,unlock}_all() functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the former use a global context stored in dev->mode_config.acquire_ctx while the latter depend on a user provided one (typically in the stack). In the old (working) code the global context structure is freed in drm_modeset_unlock_all() thus we are sure no one is holding a reference to it at that point. This means that as long as no one accesses the global dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN and unlock/END, the code should be equivalent before and after my changes. In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all() functions, the acquire_ctx field of the drm_mode_config structure should be deleted: /** * @acquire_ctx: * * Global implicit acquire context used by atomic drivers for legacy * IOCTLs. Deprecated, since implicit locking contexts make it * impossible to use driver-private &struct drm_modeset_lock. Users of * this must hold @mutex. */ struct drm_modeset_acquire_ctx *acquire_ctx; If I had done that (ie. removing this field) I would have detected the problem when compiling. There is another place (in the amdgpu driver) where this field is still being referenced, but before I investigate that I would like to know if you agree that this is a good path to follow. Regarding the second issue you raised... > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > mode_config.mutex. So would need a proper review whether we > actually need that lock or not. ...the only difference regarding mode_config.mutex I see is that in the new macros the mutex is locked only under this condition: if (!drm_drv_uses_atomic_modeset(dev)) ...which seems reasonable, right? Is this what you were referring to or is it something else? Please let me know what you think. Thanks!
On 21/10/02 09:13AM, Fernando Ramos wrote: > > Sean, could you revert the whole patch series? I'll have a deeper look into the > patch set and come up with a v3 where all these issues will be addressed. > Hi Sean, I now understand the nature of the issue that caused the problem with i915 and have proceed to remove the global context structure (which revealed a similar issue in the amdgpu driver). I have prepared a V3 version of the patch set where these issues should hopefully be fixed for both the i915 and amdgpu drivers. In order to prevent causing more disruption, could you tell me what the proper way to proceed would be? In particular: 1. Is there any place where I can push my changes so that they are tested on a i915 machine? (Some type of automated pool) 2. I can test the amdgpu driver on my machine but, what about all the other architectures? What is the standard procedure? Should I simply publish V3 and wait for feedback from the different vendors? (I would hate to cause a simular situation again) 3. Should I post V3 on top of drm-next or drm-misc-next? Thanks for your patience :)
On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > On 21/10/02 05:30AM, Ville Syrjälä wrote: > > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote: > > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > > > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along > > > > > > > with the necessary drm-tip conflict resolutions). > > > > > > > > > > > > Ugh. Did anyone actually review the locking changes this does? > > > > > > I shot the previous i915 stuff down because the commit messages > > > > > > did not address any of it. > > > > > > > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread. > > > > > > > > It was much earlir than that. > > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html > > Sorry, I'm new to this and it did not occur to me to search for similar patches > in the mailing list archives in case there were additional comments that applied > to my change set. > > In case I had done that I would have found that, as you mentioned, you had > already raised two issues back in June: > > On Tue, Jun 29, 2021, Ville Syrjälä wrote: > > > > That looks wrong. You're using a private ctx here, but still > > passing dev->mode_config.acquire_ctx to the lower level stuff. > > > > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > > mode_config.mutex. So would need a proper review whether we > > actually need that lock or not. > > The first one was pointing out the same error I would later repeat in my patch > series (ups). > > After further inspection of the code it looks to me that changing this: > > intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); > > ...into this: > > intel_modeset_setup_hw_state(dev, &ctx); > > ...would be enough. Yes. > > Why? The only difference between the old drm_modeset_{lock,unlock}_all() > functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the > former use a global context stored in dev->mode_config.acquire_ctx while the > latter depend on a user provided one (typically in the stack). > > In the old (working) code the global context structure is freed in > drm_modeset_unlock_all() thus we are sure no one is holding a reference to it at > that point. This means that as long as no one accesses the global > dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN > and unlock/END, the code should be equivalent before and after my changes. > > In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all() > functions, the acquire_ctx field of the drm_mode_config structure should be > deleted: > > /** > * @acquire_ctx: > * > * Global implicit acquire context used by atomic drivers for legacy > * IOCTLs. Deprecated, since implicit locking contexts make it > * impossible to use driver-private &struct drm_modeset_lock. Users of > * this must hold @mutex. > */ > struct drm_modeset_acquire_ctx *acquire_ctx; > > If I had done that (ie. removing this field) I would have detected the problem > when compiling. > > There is another place (in the amdgpu driver) where this field is still being > referenced, but before I investigate that I would like to know if you agree that > this is a good path to follow. Yeah, removing the mode_config.acquire_ctx is a good idea if it's no longer needed. > > Regarding the second issue you raised... > > > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > > mode_config.mutex. So would need a proper review whether we > > actually need that lock or not. > > ...the only difference regarding mode_config.mutex I see is that in the new > macros the mutex is locked only under this condition: > > if (!drm_drv_uses_atomic_modeset(dev)) > > ...which seems reasonable, right? Is this what you were referring to or is it > something else? In order to eliminate the lock one first has to determine what that lock might be protecting here, and then prove that such protection is not actually needed.
On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > > > Sean, could you revert the whole patch series? I'll have a deeper look into the > > patch set and come up with a v3 where all these issues will be addressed. > > > > Hi Sean, > > I now understand the nature of the issue that caused the problem with i915 and > have proceed to remove the global context structure (which revealed a similar > issue in the amdgpu driver). > > I have prepared a V3 version of the patch set where these issues should > hopefully be fixed for both the i915 and amdgpu drivers. > > In order to prevent causing more disruption, could you tell me what the proper > way to proceed would be? In particular: > > 1. Is there any place where I can push my changes so that they are tested > on a i915 machine? (Some type of automated pool) cc:intel-gfx, which it looks like you did, _but_ your patches did did not even apply against drm-tip so our CI rejected it. There was a reply to the patches from CI indicating that. And that is one reason I probably just ignored the whole thing. If it doesn't even apply/build it's not worth my time to read. > > 2. I can test the amdgpu driver on my machine but, what about all the other > architectures? What is the standard procedure? Should I simply publish V3 > and wait for feedback from the different vendors? (I would hate to cause a > simular situation again) > > 3. Should I post V3 on top of drm-next or drm-misc-next? The normal rule is: always work on drm-tip. That is what gets tested by our CI as well. Yes, it does mean a bit of extra hurdles during development since drm-tip is a rebasing tree, but there are tools like dim retip to help out here. As for where to merge them. I would generally recommed against merging i915 patches through drm-misc unless there is a very compelling reason to do so. i915 is a fast moving target and if there are significant changes coming in via drm-misc they usually will cause conflicts for people during drm-tip rebuild. Also I would expect to see an ack requested from i915 maintainers for merging anything significant via drm-misc, which I don't think happened in this case.
On Mon, 04 Oct 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: >> On 21/10/02 09:13AM, Fernando Ramos wrote: >> > >> > Sean, could you revert the whole patch series? I'll have a deeper look into the >> > patch set and come up with a v3 where all these issues will be addressed. >> > >> >> Hi Sean, >> >> I now understand the nature of the issue that caused the problem with i915 and >> have proceed to remove the global context structure (which revealed a similar >> issue in the amdgpu driver). >> >> I have prepared a V3 version of the patch set where these issues should >> hopefully be fixed for both the i915 and amdgpu drivers. >> >> In order to prevent causing more disruption, could you tell me what the proper >> way to proceed would be? In particular: >> >> 1. Is there any place where I can push my changes so that they are tested >> on a i915 machine? (Some type of automated pool) > > cc:intel-gfx, which it looks like you did, _but_ your patches did > did not even apply against drm-tip so our CI rejected it. There was > a reply to the patches from CI indicating that. And that is one > reason I probably just ignored the whole thing. If it doesn't > even apply/build it's not worth my time to read. > >> >> 2. I can test the amdgpu driver on my machine but, what about all the other >> architectures? What is the standard procedure? Should I simply publish V3 >> and wait for feedback from the different vendors? (I would hate to cause a >> simular situation again) >> >> 3. Should I post V3 on top of drm-next or drm-misc-next? > > The normal rule is: always work on drm-tip. That is what gets > tested by our CI as well. Yes, it does mean a bit of extra hurdles > during development since drm-tip is a rebasing tree, but there are > tools like dim retip to help out here. > > As for where to merge them. I would generally recommed against merging > i915 patches through drm-misc unless there is a very compelling reason > to do so. i915 is a fast moving target and if there are significant > changes coming in via drm-misc they usually will cause conflicts for > people during drm-tip rebuild. Also I would expect to see an ack > requested from i915 maintainers for merging anything significant via > drm-misc, which I don't think happened in this case. Indeed. All other things aside, it looks like it has enough conflict potential to warrant merging via drm-intel anyway. BR, Jani.
Hi all, One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this patch series is about. You will find two types of changes here: - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has already been done in previous commits such as b7ea04d2) - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has already been done in previous commits such as 57037094) Most of the changes are straight forward, except for a few cases in the "amd" and "i915" drivers where some extra dancing was needed to overcome the limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used once inside the same function (the reason being that the macro expansion includes *labels*, and you can not have two labels named the same inside one function) Notice that, even after this patch series, some places remain where "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present, all inside drm core (which makes sense), except for two (in "amd" and "i915") which cannot be replaced due to the way they are being used. Changes in v2: - Fix commit message typo - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible - Split drm/i915 patch into two simpler ones - Remove drm_modeset_(un)lock_all() - Fix build problems in non-x86 platforms Fernando Ramos (17): drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN() drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() part 2 drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() drm: cleanup: remove drm_modeset_(un)lock_all() doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup Documentation/gpu/todo.rst | 17 ---- Documentation/locking/ww-mutex-design.rst | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 21 +++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++----- .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 25 ++--- drivers/gpu/drm/drm_client_modeset.c | 14 ++- drivers/gpu/drm/drm_crtc_helper.c | 18 ++-- drivers/gpu/drm/drm_fb_helper.c | 10 +- drivers/gpu/drm/drm_framebuffer.c | 6 +- drivers/gpu/drm/drm_modeset_lock.c | 94 +------------------ drivers/gpu/drm/gma500/psb_device.c | 18 ++-- drivers/gpu/drm/i915/display/intel_audio.c | 16 ++-- drivers/gpu/drm/i915/display/intel_display.c | 23 ++--- .../drm/i915/display/intel_display_debugfs.c | 46 +++++---- drivers/gpu/drm/i915/display/intel_overlay.c | 46 ++++----- drivers/gpu/drm/i915/display/intel_pipe_crc.c | 7 +- drivers/gpu/drm/i915/i915_drv.c | 13 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 10 +- .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 12 +-- drivers/gpu/drm/nouveau/dispnv50/disp.c | 15 ++- drivers/gpu/drm/omapdrm/omap_fb.c | 9 +- drivers/gpu/drm/radeon/radeon_device.c | 21 +++-- drivers/gpu/drm/radeon/radeon_dp_mst.c | 10 +- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 +- drivers/gpu/drm/tegra/dsi.c | 6 +- drivers/gpu/drm/tegra/hdmi.c | 6 +- drivers/gpu/drm/tegra/sor.c | 11 ++- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++- include/drm/drm_modeset_lock.h | 2 - 30 files changed, 265 insertions(+), 292 deletions(-) base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f