Re: [PATCH v4 12/23] drm/tests: helpers: Add a (re)try helper variant to enable CRTC connector

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

Hi Maxime,

On 5/16/25 4:15 PM, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Apr 25, 2025 at 01:27:03PM +0300, Cristian Ciocaltea wrote:
>> Provide a wrapper over drm_kunit_helper_enable_crtc_connector() to
>> automatically handle EDEADLK.
>>
>> This is going to help improve the error handling in a bunch of test
>> cases without open coding the restart of the atomic sequence.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 39 +++++++++++++++++++++++++++++++
>>  include/drm/drm_kunit_helpers.h           |  7 ++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> index 5f7257840d8ef0aeabe5f00802f5037ed652ae66..4e1174c50b1f2b6358eb740cd73c6d86e53d0df9 100644
>> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> @@ -332,6 +332,45 @@ int drm_kunit_helper_enable_crtc_connector(struct kunit *test,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_kunit_helper_enable_crtc_connector);
>>  
>> +/**
>> + * drm_kunit_helper_try_enable_crtc_connector - (Re)tries to enable a CRTC -> Connector output
>> + * @test: The test context object
>> + * @drm: The device to alloc the plane for
>> + * @crtc: The CRTC to enable
>> + * @connector: The Connector to enable
>> + * @mode: The display mode to configure the CRTC with
>> + * @ctx: Locking context
>> + *
>> + * This function is a wrapper over @drm_kunit_helper_enable_crtc_connector
>> + * to automatically handle EDEADLK and (re)try to enable the route from
>> + * @crtc to @connector, with the given @mode.
>> + *
>> + * Returns:
>> + *
>> + * A pointer to the new CRTC, or an ERR_PTR() otherwise.
>> + */
>> +int drm_kunit_helper_try_enable_crtc_connector(struct kunit *test,
>> +					       struct drm_device *drm,
>> +					       struct drm_crtc *crtc,
>> +					       struct drm_connector *connector,
>> +					       const struct drm_display_mode *mode,
>> +					       struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	int ret;
>> +
>> +retry_enable:
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm, crtc, connector,
>> +						     mode, ctx);
>> +	if (ret == -EDEADLK) {
>> +		ret = drm_modeset_backoff(ctx);
>> +		if (!ret)
>> +			goto retry_enable;
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_kunit_helper_try_enable_crtc_connector);
> 
> I'm not sure it's a good idea. This function might affect the locking
> context of the caller without even reporting it.
> 
> Generally speaking, I'd really prefer to have explicit locking, even if
> it means slightly more boilerplate.

Ack.

Will drop this patch and the next one for now, and sort this out in a
separate series.

Thanks,
Cristian