| Message ID | 20250124-atomic-needs-modeset-v1-0-b0c05c9eda0f@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: introduce atomic_needs_modeset() callbacks | expand |
On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > There are several drivers which attempt to upgrading the commit to the > full mode set from their per-object atomic_check() callbacks without > calling the drm_atomic_helper_check_modeset() or > drm_atomic_helper_check() again (as requested by those functions). I don't really understand why any of that is supposedly necessary. drm_atomic_helper_check_modeset() is really all about the connector routing stuff, so if none of that is changing then there is no point in calling it again. Eg. in i915 we call it just at the start, and later on we flag internal modesets all over the place, but none of those need drm_atomic_helper_check_modeset() because nothing routing related will have changed. > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > whose only purpose is to allow the plane, connector, encoder or CRTC to > specify that for whatever reasons corresponding CRTC should undergo a > full modeset. The helpers will call these callbacks in a proper place, > adding affected objects and calling required functions as required. > > The MSM patches depend on the msm-next tree and the series at [1]. The > plan is to land core changes through drm-misc, then merge drm-misc-next > into msm-next and merge remaining msm-specific patches through the > msm-next tree. > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Dmitry Baryshkov (6): > drm/atomic-helper: add atomic_needs_modeset callbacks > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > drm/msm/dpu: use atomic_needs_modeset for CDM check > drm/msm: drop msm_atomic_check wrapper > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > drivers/gpu/drm/msm/msm_drv.h | 1 - > drivers/gpu/drm/msm/msm_kms.c | 2 +- > drivers/gpu/drm/msm/msm_kms.h | 7 --- > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > 12 files changed, 219 insertions(+), 89 deletions(-) > --- > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > Best regards, > -- > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > There are several drivers which attempt to upgrading the commit to the > > full mode set from their per-object atomic_check() callbacks without > > calling the drm_atomic_helper_check_modeset() or > > drm_atomic_helper_check() again (as requested by those functions). > > I don't really understand why any of that is supposedly necessary. > drm_atomic_helper_check_modeset() is really all about the > connector routing stuff, so if none of that is changing then there > is no point in calling it again. Eg. in i915 we call it just at > the start, and later on we flag internal modesets all over the > place, but none of those need drm_atomic_helper_check_modeset() > because nothing routing related will have changed. i915 doesn't need this because as you say, it doesn't rely on the atomic helper modeset tracking much at all, but it's all internal. This is for drivers which rely more or less entirely on drm_atomic_crtc_needs_modeset(). Also note that it's not just about connector routing, but about adding all the necessary additional states with drm_atomic_add_affected_connectors/planes and re-running all the various state computation hooks for those. Again i915 hand-rolls that all. So yeah i915 doesn't need this. -Sima > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > specify that for whatever reasons corresponding CRTC should undergo a > > full modeset. The helpers will call these callbacks in a proper place, > > adding affected objects and calling required functions as required. > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > plan is to land core changes through drm-misc, then merge drm-misc-next > > into msm-next and merge remaining msm-specific patches through the > > msm-next tree. > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > Dmitry Baryshkov (6): > > drm/atomic-helper: add atomic_needs_modeset callbacks > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > drm/msm: drop msm_atomic_check wrapper > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > 12 files changed, 219 insertions(+), 89 deletions(-) > > --- > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > Best regards, > > -- > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > -- > Ville Syrjälä > Intel
On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote: > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > > There are several drivers which attempt to upgrading the commit to the > > > full mode set from their per-object atomic_check() callbacks without > > > calling the drm_atomic_helper_check_modeset() or > > > drm_atomic_helper_check() again (as requested by those functions). > > > > I don't really understand why any of that is supposedly necessary. > > drm_atomic_helper_check_modeset() is really all about the > > connector routing stuff, so if none of that is changing then there > > is no point in calling it again. Eg. in i915 we call it just at > > the start, and later on we flag internal modesets all over the > > place, but none of those need drm_atomic_helper_check_modeset() > > because nothing routing related will have changed. > > i915 doesn't need this because as you say, it doesn't rely on the atomic > helper modeset tracking much at all, but it's all internal. This is for > drivers which rely more or less entirely on > drm_atomic_crtc_needs_modeset(). > > Also note that it's not just about connector routing, but about adding all > the necessary additional states with > drm_atomic_add_affected_connectors/planes and re-running all the various > state computation hooks for those. Again i915 hand-rolls that all. IIRC it only runs the connectors' atomic_check(), nothing else really. But maybe that's changed since I last looked at it. Anyways it feels like we're throwing everything and the kitchen sink into a single function here. Maybe it should be split into two or more functions with clear responsibilities? > > So yeah i915 doesn't need this. > -Sima > > > > > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > > specify that for whatever reasons corresponding CRTC should undergo a > > > full modeset. The helpers will call these callbacks in a proper place, > > > adding affected objects and calling required functions as required. > > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > > plan is to land core changes through drm-misc, then merge drm-misc-next > > > into msm-next and merge remaining msm-specific patches through the > > > msm-next tree. > > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > Dmitry Baryshkov (6): > > > drm/atomic-helper: add atomic_needs_modeset callbacks > > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > > drm/msm: drop msm_atomic_check wrapper > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > > 12 files changed, 219 insertions(+), 89 deletions(-) > > > --- > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > > > Best regards, > > > -- > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > -- > > Ville Syrjälä > > Intel > > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote: > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > > > There are several drivers which attempt to upgrading the commit to the > > > > full mode set from their per-object atomic_check() callbacks without > > > > calling the drm_atomic_helper_check_modeset() or > > > > drm_atomic_helper_check() again (as requested by those functions). > > > > > > I don't really understand why any of that is supposedly necessary. > > > drm_atomic_helper_check_modeset() is really all about the > > > connector routing stuff, so if none of that is changing then there > > > is no point in calling it again. Eg. in i915 we call it just at > > > the start, and later on we flag internal modesets all over the > > > place, but none of those need drm_atomic_helper_check_modeset() > > > because nothing routing related will have changed. > > > > i915 doesn't need this because as you say, it doesn't rely on the atomic > > helper modeset tracking much at all, but it's all internal. This is for > > drivers which rely more or less entirely on > > drm_atomic_crtc_needs_modeset(). > > > > Also note that it's not just about connector routing, but about adding all > > the necessary additional states with > > drm_atomic_add_affected_connectors/planes and re-running all the various > > state computation hooks for those. Again i915 hand-rolls that all. > > IIRC it only runs the connectors' atomic_check(), > nothing else really. But maybe that's changed since I last > looked at it. It calls into connector/bridge/crtc callbacks related to modesets and mode validation. The thing is a few hundred lines in total if you include all the split out subfunctions. Like the kerneldoc pretty clearly spells out that it does a lot more than what you've listed here. Just i915 doesn't used most of that. > Anyways it feels like we're throwing everything and the > kitchen sink into a single function here. Maybe it should be > split into two or more functions with clear responsibilities? I'm not sure you can split it up much, because modesetting is complicated. Like even if you'd want to split out just the routing update logic that's a pretty big mess with a bunch of callbacks so that we can pick the right encoders to add the right bridges. And then have a 2nd function that does the actual state computation/validation. Not sure that's worth it, since only benefit would be for drivers like i915 that almost entirely hand-roll their own atomic check flow and really only need the connector routing bits. I guess if you're bored you could give it a stab. -Sima > > > > > So yeah i915 doesn't need this. > > -Sima > > > > > > > > > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > > > specify that for whatever reasons corresponding CRTC should undergo a > > > > full modeset. The helpers will call these callbacks in a proper place, > > > > adding affected objects and calling required functions as required. > > > > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > > > plan is to land core changes through drm-misc, then merge drm-misc-next > > > > into msm-next and merge remaining msm-specific patches through the > > > > msm-next tree. > > > > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > Dmitry Baryshkov (6): > > > > drm/atomic-helper: add atomic_needs_modeset callbacks > > > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > > > drm/msm: drop msm_atomic_check wrapper > > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > > > 12 files changed, 219 insertions(+), 89 deletions(-) > > > > --- > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > > > > > Best regards, > > > > -- > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Simona Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel
On Fri, Jan 24, 2025 at 04:37:39PM +0100, Simona Vetter wrote: > On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote: > > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > > > > There are several drivers which attempt to upgrading the commit to the > > > > > full mode set from their per-object atomic_check() callbacks without > > > > > calling the drm_atomic_helper_check_modeset() or > > > > > drm_atomic_helper_check() again (as requested by those functions). > > > > > > > > I don't really understand why any of that is supposedly necessary. > > > > drm_atomic_helper_check_modeset() is really all about the > > > > connector routing stuff, so if none of that is changing then there > > > > is no point in calling it again. Eg. in i915 we call it just at > > > > the start, and later on we flag internal modesets all over the > > > > place, but none of those need drm_atomic_helper_check_modeset() > > > > because nothing routing related will have changed. > > > > > > i915 doesn't need this because as you say, it doesn't rely on the atomic > > > helper modeset tracking much at all, but it's all internal. This is for > > > drivers which rely more or less entirely on > > > drm_atomic_crtc_needs_modeset(). > > > > > > Also note that it's not just about connector routing, but about adding all > > > the necessary additional states with > > > drm_atomic_add_affected_connectors/planes and re-running all the various > > > state computation hooks for those. Again i915 hand-rolls that all. > > > > IIRC it only runs the connectors' atomic_check(), > > nothing else really. But maybe that's changed since I last > > looked at it. > > It calls into connector/bridge/crtc callbacks related to modesets and mode > validation. The pre-atomic mode_fixup stuff? Are people still using that in atomic drivers? Hmm, it does look like someone added some real atomic_check() calls in there, which is a slightly surprising place to find them. > > The thing is a few hundred lines in total if you include all the split out > subfunctions. Like the kerneldoc pretty clearly spells out that it does a > lot more than what you've listed here. Just i915 doesn't used most of > that. In my book a function should do one thing. And if you do have some kind of massive dispatcher function then it should be very abstract and just call some smaller functions to do each step. tldr; I don't like any function that doesn't fit on my screen. Anyways, my main worry is that someone adds some new logic/checks somewhere that assumes that you can't flag modesets later without calling the helper. Which is clearly not correct. Eg. most of the modesets we do are just done to get the hardware turned off while we reprogram some global resource that doesn't know how to synchronize with active pipes, not because anything changed that would need further checks/recomputation/etc. > > > Anyways it feels like we're throwing everything and the > > kitchen sink into a single function here. Maybe it should be > > split into two or more functions with clear responsibilities? > > I'm not sure you can split it up much, because modesetting is complicated. > Like even if you'd want to split out just the routing update logic that's > a pretty big mess with a bunch of callbacks so that we can pick the right > encoders to add the right bridges. And then have a 2nd function that does > the actual state computation/validation. > > Not sure that's worth it, since only benefit would be for drivers like > i915 that almost entirely hand-roll their own atomic check flow and really > only need the connector routing bits. I guess if you're bored you could > give it a stab. Yeah, I guess I could try to carve it up a bit when I get bored with other stuff. > -Sima > > > > > > > > > So yeah i915 doesn't need this. > > > -Sima > > > > > > > > > > > > > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > > > > specify that for whatever reasons corresponding CRTC should undergo a > > > > > full modeset. The helpers will call these callbacks in a proper place, > > > > > adding affected objects and calling required functions as required. > > > > > > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > > > > plan is to land core changes through drm-misc, then merge drm-misc-next > > > > > into msm-next and merge remaining msm-specific patches through the > > > > > msm-next tree. > > > > > > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > Dmitry Baryshkov (6): > > > > > drm/atomic-helper: add atomic_needs_modeset callbacks > > > > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > > > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > > > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > > > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > > > > drm/msm: drop msm_atomic_check wrapper > > > > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > > > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > > > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > > > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > > > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > > > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > > > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > > > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > > > > 12 files changed, 219 insertions(+), 89 deletions(-) > > > > > --- > > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > > > > > > > Best regards, > > > > > -- > > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > > > -- > > > Simona Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Ville Syrjälä > > Intel > > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote: > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > > > There are several drivers which attempt to upgrading the commit to the > > > > full mode set from their per-object atomic_check() callbacks without > > > > calling the drm_atomic_helper_check_modeset() or > > > > drm_atomic_helper_check() again (as requested by those functions). > > > > > > I don't really understand why any of that is supposedly necessary. > > > drm_atomic_helper_check_modeset() is really all about the > > > connector routing stuff, so if none of that is changing then there > > > is no point in calling it again. Eg. in i915 we call it just at > > > the start, and later on we flag internal modesets all over the > > > place, but none of those need drm_atomic_helper_check_modeset() > > > because nothing routing related will have changed. > > > > i915 doesn't need this because as you say, it doesn't rely on the atomic > > helper modeset tracking much at all, but it's all internal. This is for > > drivers which rely more or less entirely on > > drm_atomic_crtc_needs_modeset(). > > > > Also note that it's not just about connector routing, but about adding all > > the necessary additional states with > > drm_atomic_add_affected_connectors/planes and re-running all the various > > state computation hooks for those. Again i915 hand-rolls that all. > > IIRC it only runs the connectors' atomic_check(), > nothing else really. But maybe that's changed since I last > looked at it. It also calls encoder's atomic_check() or mode_fixup() callbacks (see the mode_fixup() call at the end of the helper). And the other function, drm_atomic_helper_check() calls drm_atomic_helper_check_modeset() and then calls atomic_check() for the planes and for the CRTCs that are a part of the state. For example, if the the encoder requires a modeset change on the CRTC, then that CRTC (and all connected planes) should also be added to the state. However with the default code flow it's already too late. Even if we just split those functions, it's still too late, as mode_fixup() comes after adding of the CRTC and planes. Likewise, if the plane wants to upgrade the commit, then all other planes blended on the corresponding CRTC must also be added to the state. > Anyways it feels like we're throwing everything and the > kitchen sink into a single function here. Maybe it should be > split into two or more functions with clear responsibilities? I'm not sure, what do you mean. Do you mean splitting those two drm_atomic_helper functions? It is possible, but it will also result in a higher amount of boilerplate code (and still being easy to break). For example, the mentioned depdency series adds this kind of 'oh, do we need a modeset?' call to the DPU driver. But then each other driver needs similar code. I did a quick non-comprehensive grepping (also I skipped i915 and AMDGPU drivers, too complicated to analyse): + malidp, malidp_crtc_atomic_check_gamma(), manually calls drm_atomic_helper_check_modeset() again good citizen + GUD: gud_pipe_check(), plane setting mode_changed, pardoned, single plane on a CRTC + mcde, mcde_display_check(), plane setting mode_changed pardoned, single plane on a CRTC + mgag200, mgag200_primary_plane_helper_atomic_check(), plane setting mode_changed pardoned, single plane on a CRTC + tilcdc, tilcdc_plane_atomic_check() plane setting mode_changed pardoned, single plane on a CRTC + pl111, pl111_display_check(), plane setting mode_changed pardoned, single plane on a CRTC + tve200, tve200_display_check(), plane setting mode_changed pardoned, single plane on a CRTC + IPUv3, ipu_plane_atomic_check(), plane setting mode_changed no excuse, it can come from the overlay plane + ingenic, ingenic_ipu_plane_atomic_check() plane setting mode_changed no idea + ingenic, ingenic_drm_plane_atomic_check(), plane setting mode_changed no idea + nouveau/nv50, nv50_outp_atomic_check_view(), encoder setting mode_changed no idea of the possible impact + msm, dpu_encoder_virt_atomic_check() encoder setting mode_changed possible issues once resource allocation is moved to CRTC + imx/lcdcl: imx_lcdc_pipe_update(), ignores crtc_state->mode_changed, forces mode_set on its own no excuse? needs to be refactored anyway? + meson, meson_encoder_hdmi_atomic_check() bridge setting mode_changed no idea of the possible impact + nwl-dsp, nwl_dsi_bridge_atomic_check() bridge upgrading active_changed to mode_changed no idea of the possible impact + drm_atomic_helper_connector_tv_check() connector setting mode_changed should be okay... + drm_atomic_helper_connector_hdmi_check() connector setting mode_changed should be okay... + cadence, cdns_mhdp_connector_atomic_check() connector setting mode_changed should be okay... + synopsis, dw_hdmi_connector_atomic_check connector setting mode_changed should be okay... + vc4, vc4_hdmi_connector_atomic_check() connector setting mode_changed should be okay... + vmwgfx, vmw_stdu_connector_atomic_check() connector setting mode_changed should be okay... + msm, dpu_crtc_atomic_check() CRTC upgrading active_changed to mode_changed patch pending + drm_dp_mst_add_affected_dsc_crtcs() when is it supposed to be called at all? Please note, that those 'pardoned' or 'should be okay' drivers are breaking the defined function API and might stop functioning at some point. > > > > > So yeah i915 doesn't need this. > > -Sima > > > > > > > > > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > > > specify that for whatever reasons corresponding CRTC should undergo a > > > > full modeset. The helpers will call these callbacks in a proper place, > > > > adding affected objects and calling required functions as required. > > > > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > > > plan is to land core changes through drm-misc, then merge drm-misc-next > > > > into msm-next and merge remaining msm-specific patches through the > > > > msm-next tree. > > > > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > Dmitry Baryshkov (6): > > > > drm/atomic-helper: add atomic_needs_modeset callbacks > > > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > > > drm/msm: drop msm_atomic_check wrapper > > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > > > 12 files changed, 219 insertions(+), 89 deletions(-) > > > > --- > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > > > > > Best regards, > > > > -- > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Simona Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel
On Fri, Jan 24, 2025 at 06:14:23PM +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 04:37:39PM +0100, Simona Vetter wrote: > > On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote: > > > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote: > > > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote: > > > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote: > > > > > > There are several drivers which attempt to upgrading the commit to the > > > > > > full mode set from their per-object atomic_check() callbacks without > > > > > > calling the drm_atomic_helper_check_modeset() or > > > > > > drm_atomic_helper_check() again (as requested by those functions). > > > > > > > > > > I don't really understand why any of that is supposedly necessary. > > > > > drm_atomic_helper_check_modeset() is really all about the > > > > > connector routing stuff, so if none of that is changing then there > > > > > is no point in calling it again. Eg. in i915 we call it just at > > > > > the start, and later on we flag internal modesets all over the > > > > > place, but none of those need drm_atomic_helper_check_modeset() > > > > > because nothing routing related will have changed. > > > > > > > > i915 doesn't need this because as you say, it doesn't rely on the atomic > > > > helper modeset tracking much at all, but it's all internal. This is for > > > > drivers which rely more or less entirely on > > > > drm_atomic_crtc_needs_modeset(). > > > > > > > > Also note that it's not just about connector routing, but about adding all > > > > the necessary additional states with > > > > drm_atomic_add_affected_connectors/planes and re-running all the various > > > > state computation hooks for those. Again i915 hand-rolls that all. > > > > > > IIRC it only runs the connectors' atomic_check(), > > > nothing else really. But maybe that's changed since I last > > > looked at it. > > > > It calls into connector/bridge/crtc callbacks related to modesets and mode > > validation. > > The pre-atomic mode_fixup stuff? Are people still using that in > atomic drivers? Hmm, it does look like someone added some real > atomic_check() calls in there, which is a slightly surprising > place to find them. Well for modesets you do need hooks to compute the state, and that means a modern one that passes the actual atomic state stuff as parameters. > > The thing is a few hundred lines in total if you include all the split out > > subfunctions. Like the kerneldoc pretty clearly spells out that it does a > > lot more than what you've listed here. Just i915 doesn't used most of > > that. > > In my book a function should do one thing. And if you do have some > kind of massive dispatcher function then it should be very abstract > and just call some smaller functions to do each step. > tldr; I don't like any function that doesn't fit on my screen. > > Anyways, my main worry is that someone adds some new logic/checks > somewhere that assumes that you can't flag modesets later without > calling the helper. Which is clearly not correct. Eg. most of the > modesets we do are just done to get the hardware turned off while > we reprogram some global resource that doesn't know how to > synchronize with active pipes, not because anything changed that > would need further checks/recomputation/etc. That is a bit a risk, but I think thus far all that function has done is figure out the routing and call _lots_ of callbacks on all the various objects to figure out modesets. And with connectors, bridges and crtcs (it's kidna convenient for many drivers to split the plane flip from the modeset related crtc state computations) there's just a lot of these. Beyond the routing (which could be split out I guess) I think we've succeeded at keeping random funny things out of these functions, because they are documented to be idempotent (if all your callbacks are ofc). -Sima > > > Anyways it feels like we're throwing everything and the > > > kitchen sink into a single function here. Maybe it should be > > > split into two or more functions with clear responsibilities? > > > > I'm not sure you can split it up much, because modesetting is complicated. > > Like even if you'd want to split out just the routing update logic that's > > a pretty big mess with a bunch of callbacks so that we can pick the right > > encoders to add the right bridges. And then have a 2nd function that does > > the actual state computation/validation. > > > > Not sure that's worth it, since only benefit would be for drivers like > > i915 that almost entirely hand-roll their own atomic check flow and really > > only need the connector routing bits. I guess if you're bored you could > > give it a stab. > > Yeah, I guess I could try to carve it up a bit when I get bored > with other stuff. > > > -Sima > > > > > > > > > > > > > So yeah i915 doesn't need this. > > > > -Sima > > > > > > > > > > > > > > > > > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks, > > > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to > > > > > > specify that for whatever reasons corresponding CRTC should undergo a > > > > > > full modeset. The helpers will call these callbacks in a proper place, > > > > > > adding affected objects and calling required functions as required. > > > > > > > > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The > > > > > > plan is to land core changes through drm-misc, then merge drm-misc-next > > > > > > into msm-next and merge remaining msm-specific patches through the > > > > > > msm-next tree. > > > > > > > > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ > > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > --- > > > > > > Dmitry Baryshkov (6): > > > > > > drm/atomic-helper: add atomic_needs_modeset callbacks > > > > > > drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset > > > > > > drm/msm/dpu: stop upgrading commits by enabling allow_modeset > > > > > > drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset > > > > > > drm/msm/dpu: use atomic_needs_modeset for CDM check > > > > > > drm/msm: drop msm_atomic_check wrapper > > > > > > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ > > > > > > drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + > > > > > > drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- > > > > > > drivers/gpu/drm/msm/msm_atomic.c | 29 --------- > > > > > > drivers/gpu/drm/msm/msm_drv.h | 1 - > > > > > > drivers/gpu/drm/msm/msm_kms.c | 2 +- > > > > > > drivers/gpu/drm/msm/msm_kms.h | 7 --- > > > > > > include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ > > > > > > 12 files changed, 219 insertions(+), 89 deletions(-) > > > > > > --- > > > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 > > > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 > > > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 > > > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 > > > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 > > > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf > > > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da > > > > > > > > > > > > Best regards, > > > > > > -- > > > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > > > -- > > > > Simona Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Simona Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel
There are several drivers which attempt to upgrading the commit to the full mode set from their per-object atomic_check() callbacks without calling the drm_atomic_helper_check_modeset() or drm_atomic_helper_check() again (as requested by those functions). As discussed on IRC, add separate atomic_needs_modeset() callbacks, whose only purpose is to allow the plane, connector, encoder or CRTC to specify that for whatever reasons corresponding CRTC should undergo a full modeset. The helpers will call these callbacks in a proper place, adding affected objects and calling required functions as required. The MSM patches depend on the msm-next tree and the series at [1]. The plan is to land core changes through drm-misc, then merge drm-misc-next into msm-next and merge remaining msm-specific patches through the msm-next tree. [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/ Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dmitry Baryshkov (6): drm/atomic-helper: add atomic_needs_modeset callbacks drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset drm/msm/dpu: stop upgrading commits by enabling allow_modeset drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset drm/msm/dpu: use atomic_needs_modeset for CDM check drm/msm: drop msm_atomic_check wrapper drivers/gpu/drm/drm_atomic_helper.c | 59 ++++++++++++++++++ drivers/gpu/drm/mgag200/mgag200_drv.h | 2 + drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++----- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 -------- drivers/gpu/drm/msm/msm_atomic.c | 29 --------- drivers/gpu/drm/msm/msm_drv.h | 1 - drivers/gpu/drm/msm/msm_kms.c | 2 +- drivers/gpu/drm/msm/msm_kms.h | 7 --- include/drm/drm_modeset_helper_vtables.h | 92 +++++++++++++++++++++++++++++ 12 files changed, 219 insertions(+), 89 deletions(-) --- base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6 change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7 prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2 prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6 prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7 prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da Best regards,