[PATCH] drm/simple-helpers: Always add planes to the state update

[Daniel Vetter]

On Thu, Aug 25, 2016 at 07:13:07PM +0200, Noralf Trønnes wrote:
> 
> Den 25.08.2016 08:30, skrev Daniel Vetter:
> > On Wed, Aug 24, 2016 at 08:38:37PM +0200, Noralf Trønnes wrote:
> > > Den 23.08.2016 08:25, skrev Daniel Vetter:
> > > > Our update function is hooked to the single plane, which might not get
> > > > called for crtc-only updates. Which is surprising, so fix this by
> > > > always adding the plane.
> > > > 
> > > > While at it document how&when the event should be sent out better in
> > > > the kerneldoc.
> > > > 
> > > > Cc: Noralf Trønnes <noralf at tronnes.org>
> > > > Cc: andrea.merello at gmail.com
> > > > Tested-and-Reported-by: andrea.merello at gmail.com
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > ---
> > > > I'm not entirely sure we really want to put the responsibility for
> > > > this onto driver. Plan B) would be to remove the kerneldoc I added
> > > > here and call the right function from drm_simple_kms_plane_atomic_update.
> > > > That way simple drivers don't need to deal with that detail, and in
> > > > general those drivers don't care that much about the miniscule
> > > > possible race a generic implementation would cause. What do you
> > > > suggest as the best approach?
> > > If the driver is responsible, a helper like this would be nice:
> > > 
> > > void drm_simple_display_pipe_handle_vblank_event(struct
> > > drm_simple_display_pipe *pipe)
> > > {
> > >      struct drm_crtc *crtc = &pipe->crtc;
> > > 
> > >      if (crtc->state && crtc->state->event) {
> > >          spin_lock_irq(&crtc->dev->event_lock);
> > >          drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > >          spin_unlock_irq(&crtc->dev->event_lock);
> > >          crtc->state->event = NULL;
> > For a generic version we probably want to automatically switch to
> > drm_crtc_arm_vblank_event, if vblank support is available. That can be
> > checked by looking at drm_device->num_crtcs.
> > 
> > >      }
> > > }
> > > 
> > > Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
> > > calls this, and if it is set, then the driver has to do it or something
> > > similar.
> > > 
> > > It is difficult to predict what future drivers will need.
> > > Unless there are drivers on the horizon that will need to handle the
> > > event themselves, I suggest that we follow your plan B. If the need arises,
> > > we just add a one-liner to all drivers that has the update() callback set.
> > Hm yeah. What about merging this patch here for now (would need your
> > formal reviewed-by), and then later on when we have 2-3 simple drivers
> > using this we can reconsider? Since I dont have hw for any of them I can't
> > do this myself.
> > 
> > > Noralf.
> > > 
> > > Sidenote:
> > > When I converted simpledrm I had problems with flip_done timeouts.
> > > In the end I had to check for the event in disable/enable/update.
> > > Trying it again now, it's enough to do it in update().
> > > Not sure what has changed.
> > Trying again with this patch, or without? Since this patch here is meant
> > to exactly the problem that you need to handle the event in too many
> > places. If it's fixed with just latest drm-misc, that would be surprising
> > ...
> 
> I have updated Raspian so I could use xorg to test with, and indeed
> without this patch and without sending vblank event in disable and enable,
> the flip_done timeout showed up. modetest -v didn't trigger it.
> With this patch I only have to send the vblank event in update().
> 
> Reviewed-by: Noralf Trønnes <noralf at tronnes.org>

Thanks for the review, pushed onto -misc.
-Daniel

> 
> > -Daniel
> > 
> > > > -Daniel
> > > > ---
> > > >    drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
> > > >    include/drm/drm_simple_kms_helper.h     | 6 ++++++
> > > >    2 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > index bada17166512..447631018426 100644
> > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> > > >    	.destroy = drm_encoder_cleanup,
> > > >    };
> > > > +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> > > > +				     struct drm_crtc_state *state)
> > > > +{
> > > > +	return drm_atomic_add_affected_planes(state->state, crtc);
> > > > +}
> > > > +
> > > >    static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
> > > >    {
> > > >    	struct drm_simple_display_pipe *pipe;
> > > > @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
> > > >    }
> > > >    static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> > > > +	.atomic_check = drm_simple_kms_crtc_check,
> > > >    	.disable = drm_simple_kms_crtc_disable,
> > > >    	.enable = drm_simple_kms_crtc_enable,
> > > >    };
> > > > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > > > index 269039722f91..826946ca2b82 100644
> > > > --- a/include/drm/drm_simple_kms_helper.h
> > > > +++ b/include/drm/drm_simple_kms_helper.h
> > > > @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
> > > >    	 *
> > > >    	 * This function is called when the underlying plane state is updated.
> > > >    	 * This hook is optional.
> > > > +	 *
> > > > +	 * This is the function drivers should submit the
> > > > +	 * &drm_pending_vblank_event from. Using either
> > > > +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
> > > > +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
> > > > +	 * the hardware lacks vblank support entirely.
> > > >    	 */
> > > >    	void (*update)(struct drm_simple_display_pipe *pipe,
> > > >    		       struct drm_plane_state *plane_state);
> 

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