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