Re: [PATCH v3 4/4] drm/i915/display: Error codes for async flip failures

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 22 Aug 2025, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Hey,
>
> I'm not entirely sold on the design, it's way more complicated than it
> should be, it should be trivial to add any amount of error messages.

If we add error messages, how do we ensure the error messages themselves
never become part of UAPI? It doesn't take much for someone in userspace
to start doing strcmp on them.

> Replace return -EINVAL; with return drm_atomic_error_einval(state, class, "string");

Yeah, much cleaner. And if we go this route, then maybe make it printf
style to avoid callers having to clumsily fill a buffer first.

> Where class may be an enum, but in a way more generic way than
> currently specified, for example "invalid use of api", "requires
> modeset", "invalid arguments", "driver limitations", "async flip not
> possible".

That's also easier for userspace to deal with than a plethora of
flags. I guess the goals were more ambitious wrt precision/granularity,
but is that achievable?

BR,
Jani.

> The drm_atomic_error_einval() would set class and str as appropriate,
> and then return -EINVAL.
>
> That's probably all we should need here.
>
> Kind regards,
> ~Maarten
>
> Den 2025-08-22 kl. 09:00, skrev Arun R Murthy:
>> For failures in async flip atomic check/commit path return user readable
>> error codes in struct drm_atomic_state.
>> 
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index c1a3a95c65f0b66c24ddd64f47dfdc67bbde86c9..5e23f4fc747bd01fa05eba63661bf7279b083317 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -5950,6 +5950,7 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
>>  		drm_dbg_kms(display->drm,
>>  			    "[CRTC:%d:%s] modeset required\n",
>>  			    crtc->base.base.id, crtc->base.name);
>> +		state->base.error_code->failure_flags = DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;
>>  		return -EINVAL;
>>  	}
>>  
>> @@ -6019,6 +6020,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
>>  		drm_dbg_kms(display->drm,
>>  			    "[CRTC:%d:%s] modeset required\n",
>>  			    crtc->base.base.id, crtc->base.name);
>> +		state->base.error_code->failure_flags = DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;
>>  		return -EINVAL;
>>  	}
>>  
>> @@ -6061,6 +6063,8 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
>>  				    plane->base.base.id, plane->base.name,
>>  				    &new_plane_state->hw.fb->format->format,
>>  				    new_plane_state->hw.fb->modifier);
>> +			state->base.error_code->failure_flags =
>> +				DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED;
>>  			return -EINVAL;
>>  		}
>>  
>> 
>

-- 
Jani Nikula, Intel