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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel