| Message ID | 20200528053931.29282-4-karthik.b.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Asynchronous flip implementation for i915 | expand |
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@intel.com> > --- > 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. 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. > + 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. > + 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. > + 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. 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. 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. > + 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. > + 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
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@intel.com> >> --- >> 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 >
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) { + 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) { + 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)) { + 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) { + 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); + 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)) {
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@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++++++++ 1 file changed, 55 insertions(+)