| Message ID | 20200707201229.472834-16-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence annotations, round 3 | expand |
On 07/07/2020 23:12, Daniel Vetter wrote: > Gives us proper nonblocking support for free, and a pile of other > things. The tilcdc code is simply old enough that it was never > converted over, but was stuck forever with the copypasta from when it > was initially merged. > > The riskiest thing with this conversion is maybe that there's an issue > with the vblank handling or vblank event handling, which will upset > the modern commit support in atomic helpers. But from a cursory review > drm_crtc_vblank_on/off is called in the right places, and the event > handling also seems to exist (albeit with much hand-rolling and > probably some races, could perhaps be converted over to > drm_crtc_arm_vblank_event without any real loss). > > Motivated by me not having to hand-roll the dma-fence annotations for > this. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jyri Sarha <jsarha@ti.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> I tried this out, but it is not working. Something breaks in the event handling and event reference counting. Unfortunately my vacation is pressing on, and I am not sure if I have time to debug the issue further before that. Anyway, I have attached the boot log with the following WARN dumps: ---------------------------------------------------------------- [ 12.203874] WARNING: CPU: 0 PID: 208 at drivers/gpu/drm/drm_atomic_helper.c:2329 drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] [ 12.217682] WARNING: CPU: 0 PID: 208 at drivers/gpu/drm/drm_atomic_helper.c:2329 drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] [ 232.156231] WARNING: CPU: 0 PID: 1315 at drivers/gpu/drm/drm_atomic_helper.c:2329 drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] [ 232.472068] WARNING: CPU: 0 PID: 1315 at lib/refcount.c:28 __drm_atomic_helper_plane_destroy_state+0xd0/0xe0 [drm_kms_helper] [ 240.611129] WARNING: CPU: 0 PID: 1317 at drivers/gpu/drm/drm_atomic_helper.c:2329 drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] ---------------------------------------------------------------- The first two came at boot time when setting up the fbconsole, the ones after that came when I tried to use kmstest[1]. The fbconsole came up, but nothing after that works. I am back from vacation in the beginning of august, so there may be some time before I can debug this further. Best regards, Jyri [1] https://github.com/tomba/kmsxx > --- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +---------------------------- > 1 file changed, 1 insertion(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > index 0d74a6443263..4f5fc3e87383 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > @@ -87,55 +87,10 @@ static int tilcdc_atomic_check(struct drm_device *dev, > return ret; > } > > -static int tilcdc_commit(struct drm_device *dev, > - struct drm_atomic_state *state, > - bool async) > -{ > - int ret; > - > - ret = drm_atomic_helper_prepare_planes(dev, state); > - if (ret) > - return ret; > - > - ret = drm_atomic_helper_swap_state(state, true); > - if (ret) { > - drm_atomic_helper_cleanup_planes(dev, state); > - return ret; > - } > - > - /* > - * Everything below can be run asynchronously without the need to grab > - * any modeset locks at all under one condition: It must be guaranteed > - * that the asynchronous work has either been cancelled (if the driver > - * supports it, which at least requires that the framebuffers get > - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed > - * before the new state gets committed on the software side with > - * drm_atomic_helper_swap_state(). > - * > - * This scheme allows new atomic state updates to be prepared and > - * checked in parallel to the asynchronous completion of the previous > - * update. Which is important since compositors need to figure out the > - * composition of the next frame right after having submitted the > - * current layout. > - */ > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > - > - return 0; > -} > - > static const struct drm_mode_config_funcs mode_config_funcs = { > .fb_create = drm_gem_fb_create, > .atomic_check = tilcdc_atomic_check, > - .atomic_commit = tilcdc_commit, > + .atomic_commit = drm_atomic_helper_commit, > }; > > static void modeset_init(struct drm_device *dev) >
On Wed, Jul 8, 2020 at 11:17 AM Jyri Sarha <jsarha@ti.com> wrote: > > On 07/07/2020 23:12, Daniel Vetter wrote: > > Gives us proper nonblocking support for free, and a pile of other > > things. The tilcdc code is simply old enough that it was never > > converted over, but was stuck forever with the copypasta from when it > > was initially merged. > > > > The riskiest thing with this conversion is maybe that there's an issue > > with the vblank handling or vblank event handling, which will upset > > the modern commit support in atomic helpers. But from a cursory review > > drm_crtc_vblank_on/off is called in the right places, and the event > > handling also seems to exist (albeit with much hand-rolling and > > probably some races, could perhaps be converted over to > > drm_crtc_arm_vblank_event without any real loss). > > > > Motivated by me not having to hand-roll the dma-fence annotations for > > this. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jyri Sarha <jsarha@ti.com> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > > I tried this out, but it is not working. Something breaks in the event > handling and event reference counting. Unfortunately my vacation is > pressing on, and I am not sure if I have time to debug the issue further > before that. Thanks a lot for testing, looks like tilcdc doesn't quite handle the event stuff in all cases, which results in the direct warning in hw_done, and then the refcount fallout in plane_destry_state (I think at least, not entirely sure about whether that's really just collateral damage or a 2nd bug). I'll try to come up with something, enjoy your vacations meanwhile! Cheers, Daniel > Anyway, I have attached the boot log with the following WARN dumps: > ---------------------------------------------------------------- > [ 12.203874] WARNING: CPU: 0 PID: 208 at > drivers/gpu/drm/drm_atomic_helper.c:2329 > drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] > > [ 12.217682] WARNING: CPU: 0 PID: 208 at > drivers/gpu/drm/drm_atomic_helper.c:2329 > drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] > > [ 232.156231] WARNING: CPU: 0 PID: 1315 at > drivers/gpu/drm/drm_atomic_helper.c:2329 > drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] > > [ 232.472068] WARNING: CPU: 0 PID: 1315 at lib/refcount.c:28 > __drm_atomic_helper_plane_destroy_state+0xd0/0xe0 [drm_kms_helper] > > [ 240.611129] WARNING: CPU: 0 PID: 1317 at > drivers/gpu/drm/drm_atomic_helper.c:2329 > drm_atomic_helper_commit_hw_done+0x144/0x168 [drm_kms_helper] > ---------------------------------------------------------------- > > The first two came at boot time when setting up the fbconsole, the ones > after that came when I tried to use kmstest[1]. The fbconsole came up, > but nothing after that works. > > I am back from vacation in the beginning of august, so there may be some > time before I can debug this further. > > Best regards, > Jyri > > > > [1] https://github.com/tomba/kmsxx > > > --- > > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +---------------------------- > > 1 file changed, 1 insertion(+), 46 deletions(-) > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > > index 0d74a6443263..4f5fc3e87383 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > > @@ -87,55 +87,10 @@ static int tilcdc_atomic_check(struct drm_device *dev, > > return ret; > > } > > > > -static int tilcdc_commit(struct drm_device *dev, > > - struct drm_atomic_state *state, > > - bool async) > > -{ > > - int ret; > > - > > - ret = drm_atomic_helper_prepare_planes(dev, state); > > - if (ret) > > - return ret; > > - > > - ret = drm_atomic_helper_swap_state(state, true); > > - if (ret) { > > - drm_atomic_helper_cleanup_planes(dev, state); > > - return ret; > > - } > > - > > - /* > > - * Everything below can be run asynchronously without the need to grab > > - * any modeset locks at all under one condition: It must be guaranteed > > - * that the asynchronous work has either been cancelled (if the driver > > - * supports it, which at least requires that the framebuffers get > > - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed > > - * before the new state gets committed on the software side with > > - * drm_atomic_helper_swap_state(). > > - * > > - * This scheme allows new atomic state updates to be prepared and > > - * checked in parallel to the asynchronous completion of the previous > > - * update. Which is important since compositors need to figure out the > > - * composition of the next frame right after having submitted the > > - * current layout. > > - */ > > - > > - drm_atomic_helper_commit_modeset_disables(dev, state); > > - > > - drm_atomic_helper_commit_planes(dev, state, 0); > > - > > - drm_atomic_helper_commit_modeset_enables(dev, state); > > - > > - drm_atomic_helper_wait_for_vblanks(dev, state); > > - > > - drm_atomic_helper_cleanup_planes(dev, state); > > - > > - return 0; > > -} > > - > > static const struct drm_mode_config_funcs mode_config_funcs = { > > .fb_create = drm_gem_fb_create, > > .atomic_check = tilcdc_atomic_check, > > - .atomic_commit = tilcdc_commit, > > + .atomic_commit = drm_atomic_helper_commit, > > }; > > > > static void modeset_init(struct drm_device *dev) > > > > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 0d74a6443263..4f5fc3e87383 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -87,55 +87,10 @@ static int tilcdc_atomic_check(struct drm_device *dev, return ret; } -static int tilcdc_commit(struct drm_device *dev, - struct drm_atomic_state *state, - bool async) -{ - int ret; - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - ret = drm_atomic_helper_swap_state(state, true); - if (ret) { - drm_atomic_helper_cleanup_planes(dev, state); - return ret; - } - - /* - * Everything below can be run asynchronously without the need to grab - * any modeset locks at all under one condition: It must be guaranteed - * that the asynchronous work has either been cancelled (if the driver - * supports it, which at least requires that the framebuffers get - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed - * before the new state gets committed on the software side with - * drm_atomic_helper_swap_state(). - * - * This scheme allows new atomic state updates to be prepared and - * checked in parallel to the asynchronous completion of the previous - * update. Which is important since compositors need to figure out the - * composition of the next frame right after having submitted the - * current layout. - */ - - drm_atomic_helper_commit_modeset_disables(dev, state); - - drm_atomic_helper_commit_planes(dev, state, 0); - - drm_atomic_helper_commit_modeset_enables(dev, state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); - - return 0; -} - static const struct drm_mode_config_funcs mode_config_funcs = { .fb_create = drm_gem_fb_create, .atomic_check = tilcdc_atomic_check, - .atomic_commit = tilcdc_commit, + .atomic_commit = drm_atomic_helper_commit, }; static void modeset_init(struct drm_device *dev)
Gives us proper nonblocking support for free, and a pile of other things. The tilcdc code is simply old enough that it was never converted over, but was stuck forever with the copypasta from when it was initially merged. The riskiest thing with this conversion is maybe that there's an issue with the vblank handling or vblank event handling, which will upset the modern commit support in atomic helpers. But from a cursory review drm_crtc_vblank_on/off is called in the right places, and the event handling also seems to exist (albeit with much hand-rolling and probably some races, could perhaps be converted over to drm_crtc_arm_vblank_event without any real loss). Motivated by me not having to hand-roll the dma-fence annotations for this. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jyri Sarha <jsarha@ti.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +---------------------------- 1 file changed, 1 insertion(+), 46 deletions(-)