| Message ID | a8e4cb00-5171-14e5-bbe3-dadb654ff296@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 12/12/2016 4:04 PM, Maarten Lankhorst wrote: > Do something similar to vc4, only allow updating the cursor state > in-place through a fastpath when the watermarks are unaffected. This > will allow cursor movement to be smooth, but changing cursor size or > showing/hiding cursor will still fall back so watermarks can be updated. > > Only moving and changing fb is allowed. I was implementing something similar for msm kms. I have a comment/query below. > > Changes since v1: > - Set page flip to always_unused for trybot. > - Copy fence correctly, ignore plane_state->state, should be NULL. > - Check crtc_state for !active and modeset, go to slowpath if the case. > Changes since v2: > - Make error handling work correctly. (Matthew Auld) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- > drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 2 + > 3 files changed, 163 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index dbe9fb41ae53..60d75ce8a989 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, > drm_atomic_helper_plane_destroy_state(plane, state); > } > > -static int intel_plane_atomic_check(struct drm_plane *plane, > - struct drm_plane_state *state) > +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, > + struct intel_plane_state *intel_state) > { > + struct drm_plane *plane = intel_state->base.plane; > struct drm_i915_private *dev_priv = to_i915(plane->dev); > - struct drm_crtc *crtc = state->crtc; > - struct intel_crtc *intel_crtc; > - struct intel_crtc_state *crtc_state; > + struct drm_plane_state *state = &intel_state->base; > struct intel_plane *intel_plane = to_intel_plane(plane); > - struct intel_plane_state *intel_state = to_intel_plane_state(state); > - struct drm_crtc_state *drm_crtc_state; > int ret; > > - crtc = crtc ? crtc : plane->state->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > /* > * Both crtc and plane->crtc could be NULL if we're updating a > * property while the plane is disabled. We don't actually have > * anything driver-specific we need to test in that case, so > * just return success. > */ > - if (!crtc) > + if (!intel_state->base.crtc && !plane->state->crtc) > return 0; > > - drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); > - if (WARN_ON(!drm_crtc_state)) > - return -EINVAL; > - > - crtc_state = to_intel_crtc_state(drm_crtc_state); > - > /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ > intel_state->clip.x1 = 0; > intel_state->clip.y1 = 0; > @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > return intel_plane_atomic_calc_changes(&crtc_state->base, state); > } > > +static int intel_plane_atomic_check(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct drm_crtc *crtc = state->crtc; > + struct drm_crtc_state *drm_crtc_state; > + > + crtc = crtc ? crtc : plane->state->crtc; > + > + /* > + * Both crtc and plane->crtc could be NULL if we're updating a > + * property while the plane is disabled. We don't actually have > + * anything driver-specific we need to test in that case, so > + * just return success. > + */ > + if (!crtc) > + return 0; > + > + drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); > + if (WARN_ON(!drm_crtc_state)) > + return -EINVAL; > + > + return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state), > + to_intel_plane_state(state)); > +} > + > static void intel_plane_atomic_update(struct drm_plane *plane, > struct drm_plane_state *old_state) > { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 9eaf1e5bdae9..5568ecac2edc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { > .atomic_destroy_state = intel_plane_destroy_state, > }; > > +static int > +intel_legacy_cursor_update(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + int ret; > + struct drm_plane_state *old_plane_state, *new_plane_state; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + struct drm_framebuffer *old_fb; > + struct drm_crtc_state *crtc_state = crtc->state; > + > + /* > + * When crtc is inactive or there is a modeset pending, > + * wait for it to complete in the slowpath > + */ > + if (!crtc_state->active || needs_modeset(crtc_state) || > + to_intel_crtc_state(crtc_state)->update_pipe) > + goto slow; > + > + old_plane_state = plane->state; > + > + /* > + * If any parameters change that may affect watermarks, > + * take the slowpath. Only changing fb or position should be > + * in the fastpath. > + */ > + if (old_plane_state->crtc != crtc || > + old_plane_state->src_w != src_w || > + old_plane_state->src_h != src_h || > + old_plane_state->crtc_w != crtc_w || > + old_plane_state->crtc_h != crtc_h || > + !old_plane_state->visible || > + old_plane_state->fb->modifier != fb->modifier) > + goto slow; > + > + new_plane_state = intel_plane_duplicate_state(plane); > + if (!new_plane_state) > + return -ENOMEM; About creating a new plane state here and swapping it later, I guess it's more for the sake of intel_plane_atomic_check_with_state() to go through cleanly, right? Rather than directly modifying the old plane state? I didn't see anything equivalent in vc4's implementation of update_plane, hence was wondering why exactly it is needed. Also, about updating fb in the fast path, my assumption was that it was only crtc_x/y was the thing most likely to be changed really fast. Have you seen use cases where the fb changes at a faster than vsync rate? Thanks, Archit > + > + drm_atomic_set_fb_for_plane(new_plane_state, fb); > + > + new_plane_state->src_x = src_x; > + new_plane_state->src_y = src_y; > + new_plane_state->src_w = src_w; > + new_plane_state->src_h = src_h; > + new_plane_state->crtc_x = crtc_x; > + new_plane_state->crtc_y = crtc_y; > + new_plane_state->crtc_w = crtc_w; > + new_plane_state->crtc_h = crtc_h; > + > + ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), > + to_intel_plane_state(new_plane_state)); > + if (ret) > + goto out_free; > + > + /* Visibility changed, must take slowpath. */ > + if (!new_plane_state->visible) > + goto slow_free; > + > + ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex); > + if (ret) > + goto out_free; > + > + if (INTEL_INFO(dev_priv)->cursor_needs_physical) { > + int align = IS_I830(dev_priv) ? 16 * 1024 : 256; > + > + ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align); > + if (ret) { > + DRM_DEBUG_KMS("failed to attach phys object\n"); > + goto out_unlock; > + } > + } else { > + struct i915_vma *vma; > + > + vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation); > + if (IS_ERR(vma)) { > + DRM_DEBUG_KMS("failed to pin object\n"); > + > + ret = PTR_ERR(vma); > + goto out_unlock; > + } > + } > + > + old_fb = old_plane_state->fb; > + > + i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb), > + intel_plane->frontbuffer_bit); > + > + /* Swap plane state */ > + new_plane_state->fence = old_plane_state->fence; > + *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); > + new_plane_state->fence = NULL; > + new_plane_state->fb = old_fb; > + > + intel_plane->update_plane(plane, > + to_intel_crtc_state(crtc->state), > + to_intel_plane_state(plane->state)); > + > + intel_cleanup_plane_fb(plane, new_plane_state); > + > +out_unlock: > + mutex_unlock(&dev_priv->drm.struct_mutex); > +out_free: > + intel_plane_destroy_state(plane, new_plane_state); > + return ret; > + > +slow_free: > + intel_plane_destroy_state(plane, new_plane_state); > +slow: > + return drm_atomic_helper_update_plane(plane, crtc, fb, > + crtc_x, crtc_y, crtc_w, crtc_h, > + src_x, src_y, src_w, src_h); > +} > + > +static const struct drm_plane_funcs intel_cursor_plane_funcs = { > + .update_plane = intel_legacy_cursor_update, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = intel_plane_destroy, > + .set_property = drm_atomic_helper_plane_set_property, > + .atomic_get_property = intel_plane_atomic_get_property, > + .atomic_set_property = intel_plane_atomic_set_property, > + .atomic_duplicate_state = intel_plane_duplicate_state, > + .atomic_destroy_state = intel_plane_destroy_state, > +}; > + > static struct intel_plane * > intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > { > @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > cursor->disable_plane = intel_disable_cursor_plane; > > ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base, > - 0, &intel_plane_funcs, > + 0, &intel_cursor_plane_funcs, > intel_cursor_formats, > ARRAY_SIZE(intel_cursor_formats), > DRM_PLANE_TYPE_CURSOR, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8f4ddca0f521..8b6f39506d45 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); > void intel_plane_destroy_state(struct drm_plane *plane, > struct drm_plane_state *state); > extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, > + struct intel_plane_state *intel_state); > > /* intel_color.c */ > void intel_color_init(struct drm_crtc *crtc); >
Op 13-12-16 om 14:01 schreef Archit Taneja: > Hi, > > On 12/12/2016 4:04 PM, Maarten Lankhorst wrote: >> Do something similar to vc4, only allow updating the cursor state >> in-place through a fastpath when the watermarks are unaffected. This >> will allow cursor movement to be smooth, but changing cursor size or >> showing/hiding cursor will still fall back so watermarks can be updated. >> >> Only moving and changing fb is allowed. > > I was implementing something similar for msm kms. I have a comment/query > below. > >> >> Changes since v1: >> - Set page flip to always_unused for trybot. >> - Copy fence correctly, ignore plane_state->state, should be NULL. >> - Check crtc_state for !active and modeset, go to slowpath if the case. >> Changes since v2: >> - Make error handling work correctly. (Matthew Auld) >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- >> drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_drv.h | 2 + >> 3 files changed, 163 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >> index dbe9fb41ae53..60d75ce8a989 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, >> drm_atomic_helper_plane_destroy_state(plane, state); >> } >> >> -static int intel_plane_atomic_check(struct drm_plane *plane, >> - struct drm_plane_state *state) >> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, >> + struct intel_plane_state *intel_state) >> { >> + struct drm_plane *plane = intel_state->base.plane; >> struct drm_i915_private *dev_priv = to_i915(plane->dev); >> - struct drm_crtc *crtc = state->crtc; >> - struct intel_crtc *intel_crtc; >> - struct intel_crtc_state *crtc_state; >> + struct drm_plane_state *state = &intel_state->base; >> struct intel_plane *intel_plane = to_intel_plane(plane); >> - struct intel_plane_state *intel_state = to_intel_plane_state(state); >> - struct drm_crtc_state *drm_crtc_state; >> int ret; >> >> - crtc = crtc ? crtc : plane->state->crtc; >> - intel_crtc = to_intel_crtc(crtc); >> - >> /* >> * Both crtc and plane->crtc could be NULL if we're updating a >> * property while the plane is disabled. We don't actually have >> * anything driver-specific we need to test in that case, so >> * just return success. >> */ >> - if (!crtc) >> + if (!intel_state->base.crtc && !plane->state->crtc) >> return 0; >> >> - drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); >> - if (WARN_ON(!drm_crtc_state)) >> - return -EINVAL; >> - >> - crtc_state = to_intel_crtc_state(drm_crtc_state); >> - >> /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ >> intel_state->clip.x1 = 0; >> intel_state->clip.y1 = 0; >> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, >> return intel_plane_atomic_calc_changes(&crtc_state->base, state); >> } >> >> +static int intel_plane_atomic_check(struct drm_plane *plane, >> + struct drm_plane_state *state) >> +{ >> + struct drm_crtc *crtc = state->crtc; >> + struct drm_crtc_state *drm_crtc_state; >> + >> + crtc = crtc ? crtc : plane->state->crtc; >> + >> + /* >> + * Both crtc and plane->crtc could be NULL if we're updating a >> + * property while the plane is disabled. We don't actually have >> + * anything driver-specific we need to test in that case, so >> + * just return success. >> + */ >> + if (!crtc) >> + return 0; >> + >> + drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); >> + if (WARN_ON(!drm_crtc_state)) >> + return -EINVAL; >> + >> + return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state), >> + to_intel_plane_state(state)); >> +} >> + >> static void intel_plane_atomic_update(struct drm_plane *plane, >> struct drm_plane_state *old_state) >> { >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 9eaf1e5bdae9..5568ecac2edc 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { >> .atomic_destroy_state = intel_plane_destroy_state, >> }; >> >> +static int >> +intel_legacy_cursor_update(struct drm_plane *plane, >> + struct drm_crtc *crtc, >> + struct drm_framebuffer *fb, >> + int crtc_x, int crtc_y, >> + unsigned int crtc_w, unsigned int crtc_h, >> + uint32_t src_x, uint32_t src_y, >> + uint32_t src_w, uint32_t src_h) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(crtc->dev); >> + int ret; >> + struct drm_plane_state *old_plane_state, *new_plane_state; >> + struct intel_plane *intel_plane = to_intel_plane(plane); >> + struct drm_framebuffer *old_fb; >> + struct drm_crtc_state *crtc_state = crtc->state; >> + >> + /* >> + * When crtc is inactive or there is a modeset pending, >> + * wait for it to complete in the slowpath >> + */ >> + if (!crtc_state->active || needs_modeset(crtc_state) || >> + to_intel_crtc_state(crtc_state)->update_pipe) >> + goto slow; >> + >> + old_plane_state = plane->state; >> + >> + /* >> + * If any parameters change that may affect watermarks, >> + * take the slowpath. Only changing fb or position should be >> + * in the fastpath. >> + */ >> + if (old_plane_state->crtc != crtc || >> + old_plane_state->src_w != src_w || >> + old_plane_state->src_h != src_h || >> + old_plane_state->crtc_w != crtc_w || >> + old_plane_state->crtc_h != crtc_h || >> + !old_plane_state->visible || >> + old_plane_state->fb->modifier != fb->modifier) >> + goto slow; >> + >> + new_plane_state = intel_plane_duplicate_state(plane); >> + if (!new_plane_state) >> + return -ENOMEM; > > About creating a new plane state here and swapping it later, I guess > it's more for the sake of intel_plane_atomic_check_with_state() to go > through cleanly, right? Rather than directly modifying the old plane > state? i915 keeps some derived state in intel_plane_state, we copy the full state to make sure the full derived state is updated as well. > I didn't see anything equivalent in vc4's implementation of > update_plane, hence was wondering why exactly it is needed. > > Also, about updating fb in the fast path, my assumption was > that it was only crtc_x/y was the thing most likely to be > changed really fast. Have you seen use cases where the fb > changes at a faster than vsync rate? It didn't cost much extra effort to implement fb changing too since that is handled in intel's update_plane callback too. This is why I did that as well. I'm not sure it's needed in general. Any changed fb still has to have the same size or we go to slowpath. ~Maarten
On 12/13/2016 07:22 PM, Maarten Lankhorst wrote: > Op 13-12-16 om 14:01 schreef Archit Taneja: >> Hi, >> >> On 12/12/2016 4:04 PM, Maarten Lankhorst wrote: >>> Do something similar to vc4, only allow updating the cursor state >>> in-place through a fastpath when the watermarks are unaffected. This >>> will allow cursor movement to be smooth, but changing cursor size or >>> showing/hiding cursor will still fall back so watermarks can be updated. >>> >>> Only moving and changing fb is allowed. >> >> I was implementing something similar for msm kms. I have a comment/query >> below. >> >>> >>> Changes since v1: >>> - Set page flip to always_unused for trybot. >>> - Copy fence correctly, ignore plane_state->state, should be NULL. >>> - Check crtc_state for !active and modeset, go to slowpath if the case. >>> Changes since v2: >>> - Make error handling work correctly. (Matthew Auld) >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- >>> drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- >>> drivers/gpu/drm/i915/intel_drv.h | 2 + >>> 3 files changed, 163 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >>> index dbe9fb41ae53..60d75ce8a989 100644 >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >>> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, >>> drm_atomic_helper_plane_destroy_state(plane, state); >>> } >>> >>> -static int intel_plane_atomic_check(struct drm_plane *plane, >>> - struct drm_plane_state *state) >>> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, >>> + struct intel_plane_state *intel_state) >>> { >>> + struct drm_plane *plane = intel_state->base.plane; >>> struct drm_i915_private *dev_priv = to_i915(plane->dev); >>> - struct drm_crtc *crtc = state->crtc; >>> - struct intel_crtc *intel_crtc; >>> - struct intel_crtc_state *crtc_state; >>> + struct drm_plane_state *state = &intel_state->base; >>> struct intel_plane *intel_plane = to_intel_plane(plane); >>> - struct intel_plane_state *intel_state = to_intel_plane_state(state); >>> - struct drm_crtc_state *drm_crtc_state; >>> int ret; >>> >>> - crtc = crtc ? crtc : plane->state->crtc; >>> - intel_crtc = to_intel_crtc(crtc); >>> - >>> /* >>> * Both crtc and plane->crtc could be NULL if we're updating a >>> * property while the plane is disabled. We don't actually have >>> * anything driver-specific we need to test in that case, so >>> * just return success. >>> */ >>> - if (!crtc) >>> + if (!intel_state->base.crtc && !plane->state->crtc) >>> return 0; >>> >>> - drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); >>> - if (WARN_ON(!drm_crtc_state)) >>> - return -EINVAL; >>> - >>> - crtc_state = to_intel_crtc_state(drm_crtc_state); >>> - >>> /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ >>> intel_state->clip.x1 = 0; >>> intel_state->clip.y1 = 0; >>> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, >>> return intel_plane_atomic_calc_changes(&crtc_state->base, state); >>> } >>> >>> +static int intel_plane_atomic_check(struct drm_plane *plane, >>> + struct drm_plane_state *state) >>> +{ >>> + struct drm_crtc *crtc = state->crtc; >>> + struct drm_crtc_state *drm_crtc_state; >>> + >>> + crtc = crtc ? crtc : plane->state->crtc; >>> + >>> + /* >>> + * Both crtc and plane->crtc could be NULL if we're updating a >>> + * property while the plane is disabled. We don't actually have >>> + * anything driver-specific we need to test in that case, so >>> + * just return success. >>> + */ >>> + if (!crtc) >>> + return 0; >>> + >>> + drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); >>> + if (WARN_ON(!drm_crtc_state)) >>> + return -EINVAL; >>> + >>> + return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state), >>> + to_intel_plane_state(state)); >>> +} >>> + >>> static void intel_plane_atomic_update(struct drm_plane *plane, >>> struct drm_plane_state *old_state) >>> { >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 9eaf1e5bdae9..5568ecac2edc 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { >>> .atomic_destroy_state = intel_plane_destroy_state, >>> }; >>> >>> +static int >>> +intel_legacy_cursor_update(struct drm_plane *plane, >>> + struct drm_crtc *crtc, >>> + struct drm_framebuffer *fb, >>> + int crtc_x, int crtc_y, >>> + unsigned int crtc_w, unsigned int crtc_h, >>> + uint32_t src_x, uint32_t src_y, >>> + uint32_t src_w, uint32_t src_h) >>> +{ >>> + struct drm_i915_private *dev_priv = to_i915(crtc->dev); >>> + int ret; >>> + struct drm_plane_state *old_plane_state, *new_plane_state; >>> + struct intel_plane *intel_plane = to_intel_plane(plane); >>> + struct drm_framebuffer *old_fb; >>> + struct drm_crtc_state *crtc_state = crtc->state; >>> + >>> + /* >>> + * When crtc is inactive or there is a modeset pending, >>> + * wait for it to complete in the slowpath >>> + */ >>> + if (!crtc_state->active || needs_modeset(crtc_state) || >>> + to_intel_crtc_state(crtc_state)->update_pipe) >>> + goto slow; >>> + >>> + old_plane_state = plane->state; >>> + >>> + /* >>> + * If any parameters change that may affect watermarks, >>> + * take the slowpath. Only changing fb or position should be >>> + * in the fastpath. >>> + */ >>> + if (old_plane_state->crtc != crtc || >>> + old_plane_state->src_w != src_w || >>> + old_plane_state->src_h != src_h || >>> + old_plane_state->crtc_w != crtc_w || >>> + old_plane_state->crtc_h != crtc_h || >>> + !old_plane_state->visible || >>> + old_plane_state->fb->modifier != fb->modifier) >>> + goto slow; >>> + >>> + new_plane_state = intel_plane_duplicate_state(plane); >>> + if (!new_plane_state) >>> + return -ENOMEM; >> >> About creating a new plane state here and swapping it later, I guess >> it's more for the sake of intel_plane_atomic_check_with_state() to go >> through cleanly, right? Rather than directly modifying the old plane >> state? > i915 keeps some derived state in intel_plane_state, we copy the full state to make sure the full derived state is updated as well. > >> I didn't see anything equivalent in vc4's implementation of >> update_plane, hence was wondering why exactly it is needed. >> >> Also, about updating fb in the fast path, my assumption was >> that it was only crtc_x/y was the thing most likely to be >> changed really fast. Have you seen use cases where the fb >> changes at a faster than vsync rate? > It didn't cost much extra effort to implement fb changing too since that is handled in > intel's update_plane callback too. This is why I did that as well. I'm not sure it's needed in general. > Any changed fb still has to have the same size or we go to slowpath. Okay, that make sense. Thanks, Archit > > ~Maarten >
On Mon, Dec 12, 2016 at 11:34:55AM +0100, Maarten Lankhorst wrote: > Do something similar to vc4, only allow updating the cursor state > in-place through a fastpath when the watermarks are unaffected. This > will allow cursor movement to be smooth, but changing cursor size or > showing/hiding cursor will still fall back so watermarks can be updated. > > Only moving and changing fb is allowed. > > Changes since v1: > - Set page flip to always_unused for trybot. > - Copy fence correctly, ignore plane_state->state, should be NULL. > - Check crtc_state for !active and modeset, go to slowpath if the case. > Changes since v2: > - Make error handling work correctly. (Matthew Auld) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Pulled in the entire pile through drm-misc, thanks. -Daniel > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- > drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 2 + > 3 files changed, 163 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index dbe9fb41ae53..60d75ce8a989 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, > drm_atomic_helper_plane_destroy_state(plane, state); > } > > -static int intel_plane_atomic_check(struct drm_plane *plane, > - struct drm_plane_state *state) > +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, > + struct intel_plane_state *intel_state) > { > + struct drm_plane *plane = intel_state->base.plane; > struct drm_i915_private *dev_priv = to_i915(plane->dev); > - struct drm_crtc *crtc = state->crtc; > - struct intel_crtc *intel_crtc; > - struct intel_crtc_state *crtc_state; > + struct drm_plane_state *state = &intel_state->base; > struct intel_plane *intel_plane = to_intel_plane(plane); > - struct intel_plane_state *intel_state = to_intel_plane_state(state); > - struct drm_crtc_state *drm_crtc_state; > int ret; > > - crtc = crtc ? crtc : plane->state->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > /* > * Both crtc and plane->crtc could be NULL if we're updating a > * property while the plane is disabled. We don't actually have > * anything driver-specific we need to test in that case, so > * just return success. > */ > - if (!crtc) > + if (!intel_state->base.crtc && !plane->state->crtc) > return 0; > > - drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); > - if (WARN_ON(!drm_crtc_state)) > - return -EINVAL; > - > - crtc_state = to_intel_crtc_state(drm_crtc_state); > - > /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ > intel_state->clip.x1 = 0; > intel_state->clip.y1 = 0; > @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > return intel_plane_atomic_calc_changes(&crtc_state->base, state); > } > > +static int intel_plane_atomic_check(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct drm_crtc *crtc = state->crtc; > + struct drm_crtc_state *drm_crtc_state; > + > + crtc = crtc ? crtc : plane->state->crtc; > + > + /* > + * Both crtc and plane->crtc could be NULL if we're updating a > + * property while the plane is disabled. We don't actually have > + * anything driver-specific we need to test in that case, so > + * just return success. > + */ > + if (!crtc) > + return 0; > + > + drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); > + if (WARN_ON(!drm_crtc_state)) > + return -EINVAL; > + > + return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state), > + to_intel_plane_state(state)); > +} > + > static void intel_plane_atomic_update(struct drm_plane *plane, > struct drm_plane_state *old_state) > { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 9eaf1e5bdae9..5568ecac2edc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { > .atomic_destroy_state = intel_plane_destroy_state, > }; > > +static int > +intel_legacy_cursor_update(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + int ret; > + struct drm_plane_state *old_plane_state, *new_plane_state; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + struct drm_framebuffer *old_fb; > + struct drm_crtc_state *crtc_state = crtc->state; > + > + /* > + * When crtc is inactive or there is a modeset pending, > + * wait for it to complete in the slowpath > + */ > + if (!crtc_state->active || needs_modeset(crtc_state) || > + to_intel_crtc_state(crtc_state)->update_pipe) > + goto slow; > + > + old_plane_state = plane->state; > + > + /* > + * If any parameters change that may affect watermarks, > + * take the slowpath. Only changing fb or position should be > + * in the fastpath. > + */ > + if (old_plane_state->crtc != crtc || > + old_plane_state->src_w != src_w || > + old_plane_state->src_h != src_h || > + old_plane_state->crtc_w != crtc_w || > + old_plane_state->crtc_h != crtc_h || > + !old_plane_state->visible || > + old_plane_state->fb->modifier != fb->modifier) > + goto slow; > + > + new_plane_state = intel_plane_duplicate_state(plane); > + if (!new_plane_state) > + return -ENOMEM; > + > + drm_atomic_set_fb_for_plane(new_plane_state, fb); > + > + new_plane_state->src_x = src_x; > + new_plane_state->src_y = src_y; > + new_plane_state->src_w = src_w; > + new_plane_state->src_h = src_h; > + new_plane_state->crtc_x = crtc_x; > + new_plane_state->crtc_y = crtc_y; > + new_plane_state->crtc_w = crtc_w; > + new_plane_state->crtc_h = crtc_h; > + > + ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), > + to_intel_plane_state(new_plane_state)); > + if (ret) > + goto out_free; > + > + /* Visibility changed, must take slowpath. */ > + if (!new_plane_state->visible) > + goto slow_free; > + > + ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex); > + if (ret) > + goto out_free; > + > + if (INTEL_INFO(dev_priv)->cursor_needs_physical) { > + int align = IS_I830(dev_priv) ? 16 * 1024 : 256; > + > + ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align); > + if (ret) { > + DRM_DEBUG_KMS("failed to attach phys object\n"); > + goto out_unlock; > + } > + } else { > + struct i915_vma *vma; > + > + vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation); > + if (IS_ERR(vma)) { > + DRM_DEBUG_KMS("failed to pin object\n"); > + > + ret = PTR_ERR(vma); > + goto out_unlock; > + } > + } > + > + old_fb = old_plane_state->fb; > + > + i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb), > + intel_plane->frontbuffer_bit); > + > + /* Swap plane state */ > + new_plane_state->fence = old_plane_state->fence; > + *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); > + new_plane_state->fence = NULL; > + new_plane_state->fb = old_fb; > + > + intel_plane->update_plane(plane, > + to_intel_crtc_state(crtc->state), > + to_intel_plane_state(plane->state)); > + > + intel_cleanup_plane_fb(plane, new_plane_state); > + > +out_unlock: > + mutex_unlock(&dev_priv->drm.struct_mutex); > +out_free: > + intel_plane_destroy_state(plane, new_plane_state); > + return ret; > + > +slow_free: > + intel_plane_destroy_state(plane, new_plane_state); > +slow: > + return drm_atomic_helper_update_plane(plane, crtc, fb, > + crtc_x, crtc_y, crtc_w, crtc_h, > + src_x, src_y, src_w, src_h); > +} > + > +static const struct drm_plane_funcs intel_cursor_plane_funcs = { > + .update_plane = intel_legacy_cursor_update, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = intel_plane_destroy, > + .set_property = drm_atomic_helper_plane_set_property, > + .atomic_get_property = intel_plane_atomic_get_property, > + .atomic_set_property = intel_plane_atomic_set_property, > + .atomic_duplicate_state = intel_plane_duplicate_state, > + .atomic_destroy_state = intel_plane_destroy_state, > +}; > + > static struct intel_plane * > intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > { > @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > cursor->disable_plane = intel_disable_cursor_plane; > > ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base, > - 0, &intel_plane_funcs, > + 0, &intel_cursor_plane_funcs, > intel_cursor_formats, > ARRAY_SIZE(intel_cursor_formats), > DRM_PLANE_TYPE_CURSOR, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8f4ddca0f521..8b6f39506d45 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); > void intel_plane_destroy_state(struct drm_plane *plane, > struct drm_plane_state *state); > extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, > + struct intel_plane_state *intel_state); > > /* intel_color.c */ > void intel_color_init(struct drm_crtc *crtc); > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index dbe9fb41ae53..60d75ce8a989 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); } -static int intel_plane_atomic_check(struct drm_plane *plane, - struct drm_plane_state *state) +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, + struct intel_plane_state *intel_state) { + struct drm_plane *plane = intel_state->base.plane; struct drm_i915_private *dev_priv = to_i915(plane->dev); - struct drm_crtc *crtc = state->crtc; - struct intel_crtc *intel_crtc; - struct intel_crtc_state *crtc_state; + struct drm_plane_state *state = &intel_state->base; struct intel_plane *intel_plane = to_intel_plane(plane); - struct intel_plane_state *intel_state = to_intel_plane_state(state); - struct drm_crtc_state *drm_crtc_state; int ret; - crtc = crtc ? crtc : plane->state->crtc; - intel_crtc = to_intel_crtc(crtc); - /* * Both crtc and plane->crtc could be NULL if we're updating a * property while the plane is disabled. We don't actually have * anything driver-specific we need to test in that case, so * just return success. */ - if (!crtc) + if (!intel_state->base.crtc && !plane->state->crtc) return 0; - drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); - if (WARN_ON(!drm_crtc_state)) - return -EINVAL; - - crtc_state = to_intel_crtc_state(drm_crtc_state); - /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0; @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, return intel_plane_atomic_calc_changes(&crtc_state->base, state); } +static int intel_plane_atomic_check(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_crtc *crtc = state->crtc; + struct drm_crtc_state *drm_crtc_state; + + crtc = crtc ? crtc : plane->state->crtc; + + /* + * Both crtc and plane->crtc could be NULL if we're updating a + * property while the plane is disabled. We don't actually have + * anything driver-specific we need to test in that case, so + * just return success. + */ + if (!crtc) + return 0; + + drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc); + if (WARN_ON(!drm_crtc_state)) + return -EINVAL; + + return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state), + to_intel_plane_state(state)); +} + static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9eaf1e5bdae9..5568ecac2edc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_destroy_state = intel_plane_destroy_state, }; +static int +intel_legacy_cursor_update(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int crtc_x, int crtc_y, + unsigned int crtc_w, unsigned int crtc_h, + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->dev); + int ret; + struct drm_plane_state *old_plane_state, *new_plane_state; + struct intel_plane *intel_plane = to_intel_plane(plane); + struct drm_framebuffer *old_fb; + struct drm_crtc_state *crtc_state = crtc->state; + + /* + * When crtc is inactive or there is a modeset pending, + * wait for it to complete in the slowpath + */ + if (!crtc_state->active || needs_modeset(crtc_state) || + to_intel_crtc_state(crtc_state)->update_pipe) + goto slow; + + old_plane_state = plane->state; + + /* + * If any parameters change that may affect watermarks, + * take the slowpath. Only changing fb or position should be + * in the fastpath. + */ + if (old_plane_state->crtc != crtc || + old_plane_state->src_w != src_w || + old_plane_state->src_h != src_h || + old_plane_state->crtc_w != crtc_w || + old_plane_state->crtc_h != crtc_h || + !old_plane_state->visible || + old_plane_state->fb->modifier != fb->modifier) + goto slow; + + new_plane_state = intel_plane_duplicate_state(plane); + if (!new_plane_state) + return -ENOMEM; + + drm_atomic_set_fb_for_plane(new_plane_state, fb); + + new_plane_state->src_x = src_x; + new_plane_state->src_y = src_y; + new_plane_state->src_w = src_w; + new_plane_state->src_h = src_h; + new_plane_state->crtc_x = crtc_x; + new_plane_state->crtc_y = crtc_y; + new_plane_state->crtc_w = crtc_w; + new_plane_state->crtc_h = crtc_h; + + ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), + to_intel_plane_state(new_plane_state)); + if (ret) + goto out_free; + + /* Visibility changed, must take slowpath. */ + if (!new_plane_state->visible) + goto slow_free; + + ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex); + if (ret) + goto out_free; + + if (INTEL_INFO(dev_priv)->cursor_needs_physical) { + int align = IS_I830(dev_priv) ? 16 * 1024 : 256; + + ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align); + if (ret) { + DRM_DEBUG_KMS("failed to attach phys object\n"); + goto out_unlock; + } + } else { + struct i915_vma *vma; + + vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation); + if (IS_ERR(vma)) { + DRM_DEBUG_KMS("failed to pin object\n"); + + ret = PTR_ERR(vma); + goto out_unlock; + } + } + + old_fb = old_plane_state->fb; + + i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb), + intel_plane->frontbuffer_bit); + + /* Swap plane state */ + new_plane_state->fence = old_plane_state->fence; + *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); + new_plane_state->fence = NULL; + new_plane_state->fb = old_fb; + + intel_plane->update_plane(plane, + to_intel_crtc_state(crtc->state), + to_intel_plane_state(plane->state)); + + intel_cleanup_plane_fb(plane, new_plane_state); + +out_unlock: + mutex_unlock(&dev_priv->drm.struct_mutex); +out_free: + intel_plane_destroy_state(plane, new_plane_state); + return ret; + +slow_free: + intel_plane_destroy_state(plane, new_plane_state); +slow: + return drm_atomic_helper_update_plane(plane, crtc, fb, + crtc_x, crtc_y, crtc_w, crtc_h, + src_x, src_y, src_w, src_h); +} + +static const struct drm_plane_funcs intel_cursor_plane_funcs = { + .update_plane = intel_legacy_cursor_update, + .disable_plane = drm_atomic_helper_disable_plane, + .destroy = intel_plane_destroy, + .set_property = drm_atomic_helper_plane_set_property, + .atomic_get_property = intel_plane_atomic_get_property, + .atomic_set_property = intel_plane_atomic_set_property, + .atomic_duplicate_state = intel_plane_duplicate_state, + .atomic_destroy_state = intel_plane_destroy_state, +}; + static struct intel_plane * intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) cursor->disable_plane = intel_disable_cursor_plane; ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base, - 0, &intel_plane_funcs, + 0, &intel_cursor_plane_funcs, intel_cursor_formats, ARRAY_SIZE(intel_cursor_formats), DRM_PLANE_TYPE_CURSOR, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8f4ddca0f521..8b6f39506d45 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, + struct intel_plane_state *intel_state); /* intel_color.c */ void intel_color_init(struct drm_crtc *crtc);
Do something similar to vc4, only allow updating the cursor state in-place through a fastpath when the watermarks are unaffected. This will allow cursor movement to be smooth, but changing cursor size or showing/hiding cursor will still fall back so watermarks can be updated. Only moving and changing fb is allowed. Changes since v1: - Set page flip to always_unused for trybot. - Copy fence correctly, ignore plane_state->state, should be NULL. - Check crtc_state for !active and modeset, go to slowpath if the case. Changes since v2: - Make error handling work correctly. (Matthew Auld) Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 163 insertions(+), 18 deletions(-)