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@xxxxxxxxxxxxxxx>
---
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
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
