On 6/17/2020 9:27 PM, Ville Syrjälä wrote:
On Thu, May 28, 2020 at 11:09:29AM +0530, Karthik B S wrote:
Support added only for async flips on primary plane.
If flip is requested on any other plane, reject it.
Make sure there is no change in fbc, offset and framebuffer modifiers
when async flip is requested.
If any of these are modified, reject async flip.
v2: -Replace DRM_ERROR (Paulo)
-Add check for changes in OFFSET, FBC, RC(Paulo)
v3: -Removed TODO as benchmarking tests have been run now.
Signed-off-by: Karthik B S <karthik.b.s@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index eb1c360431ae..2307f924732c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14798,6 +14798,53 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
return false;
}
+static int intel_atomic_check_async(struct intel_atomic_state *state)
+{
+ struct drm_plane *plane;
+ struct drm_plane_state *plane_state;
+ struct intel_crtc_state *old_crtc_state, *new_crtc_state;
+ struct intel_plane_state *new_plane_state, *old_plane_state;
+ struct intel_crtc *crtc;
+ struct intel_plane *intel_plane;
+ int i, j;
+
+ /*FIXME: Async flip is only supported for primary plane currently
+ * Support for overlays to be added.
+ */
+ for_each_new_plane_in_state(&state->base, plane, plane_state, j) {
+ if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
I think skl+ can do async flips on any universal planes. Earlier
platforms were limited to primary only I think. Can't remember if g4x
already had usable async flip via mmio. Pretty sure at least ilk+ had
it.
Thanks for the review.
Sure I'll update this.
Also intel_ types are preferred, so this should use those, and I
think since we're talking about hw planes we should rather check for
PLANE_PRIMARY here.
Sure, I'll change this.
+ DRM_DEBUG_KMS("Async flips is NOT supported for non-primary plane\n");
+ return -EINVAL;
+ }
+ }
+
+ for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+ new_crtc_state, i) {
+ if (old_crtc_state->enable_fbc != new_crtc_state->enable_fbc) {
enable_fbc is bork, so this probably doesn't do anything particularly
sensible.
Sure. I'll remove this.
+ DRM_DEBUG_KMS("FBC status cannot be changed in async flip\n");
+ return -EINVAL;
+ }
+ }
+
+ for_each_oldnew_intel_plane_in_state(state, intel_plane, old_plane_state,
+ new_plane_state, i) {
+ if ((old_plane_state->color_plane[0].x !=
+ new_plane_state->color_plane[0].x) ||
+ (old_plane_state->color_plane[0].y !=
+ new_plane_state->color_plane[0].y)) {
Don't think we've even calculated those by the time you call this. So
this stuff has to be called much later I think.
Sure. I'll check this and move it to the right place.
+ DRM_DEBUG_KMS("Offsets cannot be changed in async\n");
+ return -EINVAL;
+ }
+
+ if (old_plane_state->uapi.fb->modifier !=
+ new_plane_state->uapi.fb->modifier) {
We seem to be missing a lot of state here. Basically I think async flip
can *only* change the plane surface address, so anything else changing
we should reject. I guess if this comes in via the legacy page flip path
the code/helpers do prevent most other things changing, but not sure.
I don't really like relying on such core checks since someone could
blindly expose this via the atomic ioctl without having those same
restrictions in place.
Yes. I have not added the checks which were present in the legacy page
flip path. Does it mean that I should add those checks also in here? Or
Am I missing something in understanding the comment? Is there any other
way to make sure only the plane surface address is changing.
We might also want a dedicated plane hook for async flips since writing
all the plane registers for these is rather pointless. I'm not even sure
what happens with all the other double buffered registers if you write
them and then do an async surface address update.
Sure. I'll make a dedicated plane hook for async flips so that we only
update the Surface address register here.
Also if we want more accurate timestmaps based on the flip timestamp
register then we're going to have to limit async flips to single plane
per pipe at a time becasue the timestamp can only be sampled from
a single plane.
Sure. I'll make sure async flips are limited to a single plane per pipe.
+ DRM_DEBUG_KMS("Framebuffer modifiers cannot be changed in async flip\n");
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
+
/**
* intel_atomic_check - validate state object
* @dev: drm device
@@ -14825,6 +14872,14 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
+ for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+ if (new_crtc_state->uapi.async_flip) {
+ ret = intel_atomic_check_async(state);
Kinda redundant to call this multiple times. I think the async_flip flag
is actually misplaced. It should probably be in the drm_atomic_state
instead of the crtc state.
Also still not a huge fan of using the "async flip" termonology in
the drm core. IMO we should just adopt the vulkan terminology for
this stuff so it's obviuos what people mean when they talk about these
things.
A little lost here.Could you please help me out? I should move the
async_flip flag to drm_atomic_state from crtc_state and then change its
name? What would be the proper vulkan terminology for this?
Thanks and Regards,
Karthik.B.S
+ if (ret)
+ goto fail;
+ }
+ }
+
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if (!needs_modeset(new_crtc_state)) {
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx