Re: [Intel-gfx] [PATCH v3 3/5] drm/i915: Add checks specific to async flips

[Karthik B S <karthik.b.s@intel.com>]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx