RE: [PATCH] drm/atomic: Add target_vblank support in atomic helpers.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, January 04, 2017 3:44 AM
> To: Grodzovsky, Andrey
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; Deucher,
> Alexander; Daenzer, Michel; Wentland, Harry
> Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> helpers.
> 
> On Tue, Jan 03, 2017 at 10:06:38AM -0500, Andrey Grodzovsky wrote:
> > Allows usage of the new page_flip_target hook for drivers implementing
> > the atomic path.
> > Provides default atomic helper for the new hook.
> >
> > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx>
> 
> Please keep a per-patch changelog so that it's easier for everyone to follow
> the evolution of this patch.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 146
> ++++++++++++++++++++++++++++--------
> >  include/drm/drm_atomic_helper.h     |   6 ++
> >  include/drm/drm_crtc.h              |   3 +
> >  3 files changed, 125 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 583f47f..5e76f90 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2733,6 +2733,59 @@ int drm_atomic_helper_resume(struct
> drm_device
> > *dev,  }  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >
> > +static int drm_atomic_helper_page_flip_create_state(
> 
> I'd just call this page_flip_common without the long namespace prefix since
> it's static. And without the create_state name since this function also does
> some basic checks.
> 
> > +				struct drm_atomic_state *state,
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event) {
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_plane_state *plane_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	crtc_state->event = event;
> > +
> > +	plane_state = drm_atomic_get_plane_state(state, plane);
> > +	if (IS_ERR(plane_state))
> > +		return PTR_ERR(plane_state);
> > +
> > +
> > +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +	if (ret != 0)
> > +		return ret;
> > +	drm_atomic_set_fb_for_plane(plane_state, fb);
> > +
> > +	/* Make sure we don't accidentally do a full modeset. */
> > +	state->allow_modeset = false;
> > +	if (!crtc_state->active) {
> > +		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy
> flip\n",
> > +				 crtc->base.id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void drm_atomic_helper_page_flip_prepare_retry(
> > +				struct drm_atomic_state *state,
> > +				struct drm_plane *plane)
> > +{
> > +	drm_atomic_state_clear(state);
> > +	drm_atomic_legacy_backoff(state);
> > +
> > +	/*
> > +	 * Someone might have exchanged the framebuffer while we
> dropped locks
> > +	 * in the backoff code. We need to fix up the fb refcount tracking the
> > +	 * core does for us.
> > +	 */
> > +	plane->old_fb = plane->fb;
> 
> There's a lot more places where we do this trick, please either refactor them
> all, or none. Personally I'd go with none since the function call is more
> verbose than the assignement, and you're hiding this rather important
> comment behind a function name that doesn't say what games are being
> played here.
> 
> > +}
> > +
> >  /**
> >   * drm_atomic_helper_page_flip - execute a legacy page flip
> >   * @crtc: DRM crtc
> > @@ -2756,8 +2809,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> > *crtc,  {
> >  	struct drm_plane *plane = crtc->primary;
> >  	struct drm_atomic_state *state;
> > -	struct drm_plane_state *plane_state;
> > -	struct drm_crtc_state *crtc_state;
> >  	int ret = 0;
> >
> >  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC) @@ -2768,35 +2819,79
> @@ int
> > drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> >  		return -ENOMEM;
> >
> >  	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> > +
> >  retry:
> > -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > -	if (IS_ERR(crtc_state)) {
> > -		ret = PTR_ERR(crtc_state);
> > +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb,
> event);
> > +	if (ret != 0)
> >  		goto fail;
> > -	}
> > -	crtc_state->event = event;
> >
> > -	plane_state = drm_atomic_get_plane_state(state, plane);
> > -	if (IS_ERR(plane_state)) {
> > -		ret = PTR_ERR(plane_state);
> > -		goto fail;
> > -	}
> > +	ret = drm_atomic_nonblocking_commit(state);
> >
> > -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +fail:
> > +	if (ret == -EDEADLK)
> > +		goto backoff;
> > +
> > +	drm_atomic_state_put(state);
> > +	return ret;
> > +
> > +backoff:
> > +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> > +	goto retry;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +
> > +/**
> > + * drm_atomic_helper_page_flip - execute a legacy page flip
> > + * @crtc: DRM crtc
> > + * @fb: DRM framebuffer
> > + * @event: optional DRM event to signal upon completion
> > + * @flags: flip flags for non-vblank sync'ed updates
> > + *
> > + * Provides a default page flip on specific vblank implementation
> > +using
> > + * the atomic driver interface.
> > + *
> > + * Note that for now so called async page flips (i.e. updates which
> > +are not
> > + * synchronized to vblank) are not supported, since the atomic
> > +interfaces have
> > + * no provisions for this yet.
> > + *
> > + * Returns:
> > + * Returns 0 on success, negative errno numbers on failure.
> 
> Kerneldoc isn't updated, just verbatim copypaste. Please also linke these
> 2 functions. And if you have time, would be good to sprinkle links to the

Not sure here, do you mean kerneldoc is not generated for the new helper function ? If so ,
Is it because @tags are missing ?

> vfunc hooks for each of these default helpers (they're unfortunately missing
> ...), e.g. here &drm_crtc_funcs.page_flip_target.

Do you mean adding kernedoc for the vfunc hook ?

Thanks, Andrey
> 
> And please build the docs per http://blog.ffwll.ch/2016/12/howto-docs.html
> and make sure it all looks pretty.
> 
> > + */
> > +int drm_atomic_helper_page_flip_on_target_vblank(
> 
> Please name this helper to match the vhook it's supposed to slot into, i.e.
> drm_atomic_helper_page_flip_target.
> 
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_atomic_state *state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > +		return -EINVAL;
> > +
> > +	state = drm_atomic_state_alloc(plane->dev);
> > +	if (!state)
> > +		return -ENOMEM;
> > +
> > +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> > +
> > +retry:
> > +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb,
> > +event);
> >  	if (ret != 0)
> >  		goto fail;
> > -	drm_atomic_set_fb_for_plane(plane_state, fb);
> >
> > -	/* Make sure we don't accidentally do a full modeset. */
> > -	state->allow_modeset = false;
> > -	if (!crtc_state->active) {
> > -		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy
> flip\n",
> > -				 crtc->base.id);
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> > +	if (WARN_ON(!crtc_state)) {
> >  		ret = -EINVAL;
> >  		goto fail;
> >  	}
> > +	crtc_state->target_vblank = target;
> >
> >  	ret = drm_atomic_nonblocking_commit(state);
> > +
> >  fail:
> >  	if (ret == -EDEADLK)
> >  		goto backoff;
> > @@ -2805,19 +2900,10 @@ int drm_atomic_helper_page_flip(struct
> drm_crtc *crtc,
> >  	return ret;
> >
> >  backoff:
> > -	drm_atomic_state_clear(state);
> > -	drm_atomic_legacy_backoff(state);
> > -
> > -	/*
> > -	 * Someone might have exchanged the framebuffer while we
> dropped locks
> > -	 * in the backoff code. We need to fix up the fb refcount tracking the
> > -	 * core does for us.
> > -	 */
> > -	plane->old_fb = plane->fb;
> > -
> > +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> >  	goto retry;
> >  }
> > -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip_on_target_vblank);
> >
> >  /**
> >   * drm_atomic_helper_connector_dpms() - connector dpms helper
> > implementation diff --git a/include/drm/drm_atomic_helper.h
> > b/include/drm/drm_atomic_helper.h index 7ff92b0..e8cb6b7 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >  				struct drm_framebuffer *fb,
> >  				struct drm_pending_vblank_event *event,
> >  				uint32_t flags);
> > +int drm_atomic_helper_page_flip_on_target_vblank(
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target);
> >  int drm_atomic_helper_connector_dpms(struct drm_connector
> *connector,
> >  				     int mode);
> >  struct drm_encoder *
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> > 946672f..cc926dc 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -103,6 +103,7 @@ static inline uint64_t I642U64(int64_t val)
> >   * @ctm: Transformation matrix
> >   * @gamma_lut: Lookup table for converting pixel data after the
> >   *	conversion matrix
> > + * @target_vblank: Target vblank count to flip
> >   * @state: backpointer to global drm_atomic_state
> >   *
> >   * Note that the distinction between @enable and @active is rather
> subtile:
> > @@ -156,6 +157,8 @@ struct drm_crtc_state {
> >  	struct drm_property_blob *ctm;
> >  	struct drm_property_blob *gamma_lut;
> >
> > +	u32 target_vblank;
> 
> The in-line style is preferred for structures, since it allows to group comments
> with each member. There's also still the question of how drivers opt-in into
> target_vblank (only amdgpu, and then only in your private branch supports
> it). I think that should be documented in the kernel-doc.
> 
> Thanks, Daniel
> > +
> >  	/**
> >  	 * @event:
> >  	 *
> > --
> > 1.9.1
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux