[Intel-gfx] [PATCH] drm/tilcdc: Use standard drm_atomic_helper_commit
Daniel Vetter
daniel at ffwll.ch
Tue Jul 14 08:32:21 UTC 2020
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 at intel.com>
> > Cc: Jyri Sarha <jsarha at ti.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
>
> Tested-by: Jyri Sarha <jsarha at ti.com>
> Reviewed-by: Jyri Sarha <jsarha at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list