[Intel-gfx] [PATCH v6 3/7] drm/i915: Add checks specific to async flips

[Karthik B S]

On 9/1/2020 4:51 PM, Ville Syrjälä wrote:
> On Fri, Aug 07, 2020 at 03:05:47PM +0530, Karthik B S wrote:
>> 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.
>>
>> v4: -Added more state checks for async flip (Ville)
>>      -Moved intel_atomic_check_async to the end of intel_atomic_check
>>       as the plane checks needs to pass before this. (Ville)
>>      -Removed crtc_state->enable_fbc check. (Ville)
>>      -Set the I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP flag for async
>>       flip case as scanline counter is not reliable here.
>>
>> v5: -Fix typo and other check patch errors seen in CI
>>       in 'intel_atomic_check_async' function.
>>
>> v6: -Don't call intel_atomic_check_async multiple times. (Ville)
>>      -Remove the check for n_planes in intel_atomic_check_async
>>      -Added documentation for async flips. (Paulo)
>>
>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 113 +++++++++++++++++++
>>   1 file changed, 113 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index ce2b0c14a073..9629c751d2af 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -14832,6 +14832,110 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
>>   	return false;
>>   }
>>   
>> +/**
>> + * DOC: asynchronous flip implementation
>> + *
>> + * Asynchronous page flip is the implementation for the DRM_MODE_PAGE_FLIP_ASYNC
>> + * flag. Currently async flip is only supported via the drmModePageFlip IOCTL.
>> + * Correspondingly, support is currently added for primary plane only.
>> + *
>> + * Async flip can only change the plane surface address, so anything else
>> + * changing is rejected from the intel_atomic_check_async() function.
>> + * Once this check is cleared, flip done interrupt is enabled using
>> + * the skl_enable_flip_done() function.
>> + *
>> + * As soon as the surface address register is written, flip done interrupt is
>> + * generated and the requested events are sent to the usersapce in the interrupt
>> + * handler itself. The timestamp and sequence sent during the flip done event
>> + * correspond to the last vblank and have no relation to the actual time when
>> + * the flip done event was sent.
>> + */
>> +
>> +static int intel_atomic_check_async(struct intel_atomic_state *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;
> 
> s/intel_plane/plane/
> 

Thanks for the review.
I will update this.
>> +	int i;
>> +
>> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>> +					    new_crtc_state, i) {
>> +		if (needs_modeset(new_crtc_state)) {
>> +			DRM_DEBUG_KMS("Modeset Required. Async flip not supported\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (!new_crtc_state->uapi.active) {
>> +			DRM_DEBUG_KMS("CRTC inactive\n");
>> +			return -EINVAL;
>> +		}
> 
> All the uapi.foo stuff should be hw.foo most likely.
>
Will update this.

>> +		if (old_crtc_state->active_planes != new_crtc_state->active_planes) {
>> +			DRM_DEBUG_KMS("Active planes cannot be changed during async flip\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	for_each_oldnew_intel_plane_in_state(state, intel_plane, old_plane_state,
>> +					     new_plane_state, i) {
>> +		/*TODO: Async flip is only supported through the page flip IOCTL
>> +		 * as of now. So support currently added for primary plane only.
>> +		 * Support for other planes should be added when async flip is
>> +		 * enabled in the atomic IOCTL path.
>> +		 */
>> +		if (intel_plane->id != PLANE_PRIMARY)
>> +			return -EINVAL;
>> +
>> +		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 flip\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;
>> +		}
>> +
>> +		if (old_plane_state->uapi.fb->format !=
>> +		    new_plane_state->uapi.fb->format) {
>> +			DRM_DEBUG_KMS("Framebuffer format cannot be changed in async flip\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (intel_wm_need_update(old_plane_state, new_plane_state)) {
> 
> That function is meant for pre-g4x wm hacks only. Do not use.
> 

Sure. Will put the checks present in the intel_wm_need_update function 
explicitly here.
>> +			DRM_DEBUG_KMS("WM update not allowed in async flip\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (old_plane_state->uapi.alpha != new_plane_state->uapi.alpha) {
>> +			DRM_DEBUG_KMS("Alpha value cannot be changed in async flip\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (old_plane_state->uapi.pixel_blend_mode !=
>> +		    new_plane_state->uapi.pixel_blend_mode) {
>> +			DRM_DEBUG_KMS("Pixel blend mode cannot be changed in async flip\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (old_plane_state->uapi.color_encoding != new_plane_state->uapi.color_encoding) {
>> +			DRM_DEBUG_KMS("Color encoding cannot be changed in async flip\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (old_plane_state->uapi.color_range != new_plane_state->uapi.color_range) {
>> +			DRM_DEBUG_KMS("Color range cannot be changed in async flip\n");
>> +			return -EINVAL;
>> +		}
> 
> Seems to be missing at least a dst coordinate check.
> 

Sure, will add this. I hadn't added it as it would be covered in the 
previous wm check. Will update it.
> Not sure we can allow async flip with linear buffers. Older hw at least
> had issues with that.
> 

I did not find any restrictions regarding this on bspec.
 From the bspec, "Changes to stride, pixel, format, RenderCompression, 
FBC, etc. are not allowed"
In IGT, we're currently using x-tiled buffer. Will confirm this via IGT 
and if required add a check for this.
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * intel_atomic_check - validate state object
>>    * @dev: drm device
>> @@ -15011,6 +15115,15 @@ static int intel_atomic_check(struct drm_device *dev,
>>   				       "[modeset]" : "[fastset]");
>>   	}
>>   
>> +	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;
>> +
>> +			break;
> 
> Why would we break here?
> 

Calling this multiple times is redundant, so put a break here.
Would you suggest handling this differently?

Thanks,
Karthik.B.S
>> +		}
>> +	}
>>   	return 0;
>>   
>>    fail:
>> -- 
>> 2.22.0
>