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.ciocal...@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
