Re: [PATCH 15/25] drm/tilcdc: Use standard drm_atomic_helper_commit

[Daniel Vetter <daniel.vetter@ffwll.ch>]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch