| Message ID | 20200708142050.530240-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/tilcdc: Use standard drm_atomic_helper_commit | expand |
Thank you Daniel, Now this works perfectly, all while I was on vacation. On 08/07/2020 17:20, 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. > > v2: Clear out crtc_state->event when we're handling the event, to > avoid upsetting the helpers (reported by Jyri). > > v3: Also send out even whent the crtc is getting disabled. Tilcdc looks a > bit like conversion to simple display helpers would work out really > nice. > Probably. Should take a closer looks some day when I have time. > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jyri Sarha <jsarha@ti.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> Tested-by: Jyri Sarha <jsarha@ti.com> Reviewed-by: Jyri Sarha <jsarha@ti.com> > -- > From logs looks like we're not stuck when disabling the display, so I > hacked in a bit of code for that too. Like mentioned above, tilcdc > looks like a perfect candidate for simple display helpers, I think > that would simplify a _lot_ of code here. > -Daniel > --- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 13 ++++++++ > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +-------------------------- > drivers/gpu/drm/tilcdc/tilcdc_plane.c | 8 +++-- > 3 files changed, 19 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index e9dd5e5cb4e7..1856962411c7 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -537,6 +537,18 @@ static void tilcdc_crtc_atomic_disable(struct drm_crtc *crtc, > tilcdc_crtc_disable(crtc); > } > > +static void tilcdc_crtc_atomic_flush(struct drm_crtc *crtc, > + struct drm_crtc_state *old_state) > +{ > + if (!crtc->state->event) > + return; > + > + spin_lock_irq(&crtc->dev->event_lock); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + crtc->state->event = NULL; > + spin_unlock_irq(&crtc->dev->event_lock); > +} > + > void tilcdc_crtc_shutdown(struct drm_crtc *crtc) > { > tilcdc_crtc_off(crtc, true); > @@ -822,6 +834,7 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = { > .atomic_check = tilcdc_crtc_atomic_check, > .atomic_enable = tilcdc_crtc_atomic_enable, > .atomic_disable = tilcdc_crtc_atomic_disable, > + .atomic_flush = tilcdc_crtc_atomic_flush, > }; > > void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc, > 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) > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c > index 0d09b31ae759..2f681a713815 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c > @@ -83,9 +83,11 @@ static void tilcdc_plane_atomic_update(struct drm_plane *plane, > if (WARN_ON(!state->fb || !state->crtc->state)) > return; > > - tilcdc_crtc_update_fb(state->crtc, > - state->fb, > - state->crtc->state->event); > + if (tilcdc_crtc_update_fb(state->crtc, > + state->fb, > + state->crtc->state->event) == 0) { > + state->crtc->state->event = NULL; > + } > } > > static const struct drm_plane_helper_funcs plane_helper_funcs = { >
On Fri, Jul 10, 2020 at 02:16:50PM +0300, Jyri Sarha wrote: > Thank you Daniel, > Now this works perfectly, all while I was on vacation. > > On 08/07/2020 17:20, 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. > > > > v2: Clear out crtc_state->event when we're handling the event, to > > avoid upsetting the helpers (reported by Jyri). > > > > v3: Also send out even whent the crtc is getting disabled. Tilcdc looks a > > bit like conversion to simple display helpers would work out really > > nice. > > > > Probably. Should take a closer looks some day when I have time. > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jyri Sarha <jsarha@ti.com> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Tested-by: Jyri Sarha <jsarha@ti.com> > Reviewed-by: Jyri Sarha <jsarha@ti.com> Thanks for testing and reviewing, patch pushed to drm-misc-next. -Daniel > > > -- > > From logs looks like we're not stuck when disabling the display, so I > > hacked in a bit of code for that too. Like mentioned above, tilcdc > > looks like a perfect candidate for simple display helpers, I think > > that would simplify a _lot_ of code here. > > -Daniel > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 13 ++++++++ > > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +-------------------------- > > drivers/gpu/drm/tilcdc/tilcdc_plane.c | 8 +++-- > > 3 files changed, 19 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > index e9dd5e5cb4e7..1856962411c7 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > @@ -537,6 +537,18 @@ static void tilcdc_crtc_atomic_disable(struct drm_crtc *crtc, > > tilcdc_crtc_disable(crtc); > > } > > > > +static void tilcdc_crtc_atomic_flush(struct drm_crtc *crtc, > > + struct drm_crtc_state *old_state) > > +{ > > + if (!crtc->state->event) > > + return; > > + > > + spin_lock_irq(&crtc->dev->event_lock); > > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > > + crtc->state->event = NULL; > > + spin_unlock_irq(&crtc->dev->event_lock); > > +} > > + > > void tilcdc_crtc_shutdown(struct drm_crtc *crtc) > > { > > tilcdc_crtc_off(crtc, true); > > @@ -822,6 +834,7 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = { > > .atomic_check = tilcdc_crtc_atomic_check, > > .atomic_enable = tilcdc_crtc_atomic_enable, > > .atomic_disable = tilcdc_crtc_atomic_disable, > > + .atomic_flush = tilcdc_crtc_atomic_flush, > > }; > > > > void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc, > > 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) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c > > index 0d09b31ae759..2f681a713815 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c > > @@ -83,9 +83,11 @@ static void tilcdc_plane_atomic_update(struct drm_plane *plane, > > if (WARN_ON(!state->fb || !state->crtc->state)) > > return; > > > > - tilcdc_crtc_update_fb(state->crtc, > > - state->fb, > > - state->crtc->state->event); > > + if (tilcdc_crtc_update_fb(state->crtc, > > + state->fb, > > + state->crtc->state->event) == 0) { > > + state->crtc->state->event = NULL; > > + } > > } > > > > static const struct drm_plane_helper_funcs plane_helper_funcs = { > > > > > -- > 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_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index e9dd5e5cb4e7..1856962411c7 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -537,6 +537,18 @@ static void tilcdc_crtc_atomic_disable(struct drm_crtc *crtc, tilcdc_crtc_disable(crtc); } +static void tilcdc_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_state) +{ + if (!crtc->state->event) + return; + + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; + spin_unlock_irq(&crtc->dev->event_lock); +} + void tilcdc_crtc_shutdown(struct drm_crtc *crtc) { tilcdc_crtc_off(crtc, true); @@ -822,6 +834,7 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = { .atomic_check = tilcdc_crtc_atomic_check, .atomic_enable = tilcdc_crtc_atomic_enable, .atomic_disable = tilcdc_crtc_atomic_disable, + .atomic_flush = tilcdc_crtc_atomic_flush, }; void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc, 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) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c index 0d09b31ae759..2f681a713815 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c @@ -83,9 +83,11 @@ static void tilcdc_plane_atomic_update(struct drm_plane *plane, if (WARN_ON(!state->fb || !state->crtc->state)) return; - tilcdc_crtc_update_fb(state->crtc, - state->fb, - state->crtc->state->event); + if (tilcdc_crtc_update_fb(state->crtc, + state->fb, + state->crtc->state->event) == 0) { + state->crtc->state->event = NULL; + } } static const struct drm_plane_helper_funcs plane_helper_funcs = {
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. v2: Clear out crtc_state->event when we're handling the event, to avoid upsetting the helpers (reported by Jyri). v3: Also send out even whent the crtc is getting disabled. Tilcdc looks a bit like conversion to simple display helpers would work out really nice. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jyri Sarha <jsarha@ti.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> -- From logs looks like we're not stuck when disabling the display, so I hacked in a bit of code for that too. Like mentioned above, tilcdc looks like a perfect candidate for simple display helpers, I think that would simplify a _lot_ of code here. -Daniel --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 13 ++++++++ drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +-------------------------- drivers/gpu/drm/tilcdc/tilcdc_plane.c | 8 +++-- 3 files changed, 19 insertions(+), 49 deletions(-)