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

[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
...
-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