[v4] drm/connector: hdmi: Allow using the YUV420 output format

[PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Cristian Ciocaltea 9 months, 2 weeks ago

In preparation to improve error handling throughout all test cases,
introduce a macro to check for EDEADLK and automate the restart of the
atomic sequence.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
 				test_edid_hdmi_1080p_rgb_max_200mhz);
 }
 
+#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
+	if (ret == -EDEADLK) {							\
+		if (state)							\
+			drm_atomic_state_clear(state);				\
+		ret = drm_modeset_backoff(ctx);					\
+		if (!ret)							\
+			goto start;						\
+	}									\
+} while (0)
+
 /*
  * Test that if we change the RGB quantization property to a different
  * value, we trigger a mode change on the connector's CRTC, which will

-- 
2.49.0

Re: [PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Maxime Ripard 8 months, 3 weeks ago

Hi,

On Fri, Apr 25, 2025 at 01:27:05PM +0300, Cristian Ciocaltea wrote:
> In preparation to improve error handling throughout all test cases,
> introduce a macro to check for EDEADLK and automate the restart of the
> atomic sequence.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
>  				test_edid_hdmi_1080p_rgb_max_200mhz);
>  }
>  
> +#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
> +	if (ret == -EDEADLK) {							\
> +		if (state)							\
> +			drm_atomic_state_clear(state);				\
> +		ret = drm_modeset_backoff(ctx);					\
> +		if (!ret)							\
> +			goto start;						\
> +	}									\
> +} while (0)
> +

I'm not sure here either, for pretty much the same reason. As far as
locking goes, I really think we should prefer something explicit even if
it means a bit more boilerplate.

If you still want to push this forward though, this has nothing to do
with kunit so it should be made a common helper. I do think it should be
done in a separate series though. Ever-expanding series are a nightmare,
both to contribute and to review :)

Maxime

Re: [PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Cristian Ciocaltea 8 months, 3 weeks ago

On 5/19/25 10:22 AM, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Apr 25, 2025 at 01:27:05PM +0300, Cristian Ciocaltea wrote:
>> In preparation to improve error handling throughout all test cases,
>> introduce a macro to check for EDEADLK and automate the restart of the
>> atomic sequence.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
>>  				test_edid_hdmi_1080p_rgb_max_200mhz);
>>  }
>>  
>> +#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
>> +	if (ret == -EDEADLK) {							\
>> +		if (state)							\
>> +			drm_atomic_state_clear(state);				\
>> +		ret = drm_modeset_backoff(ctx);					\
>> +		if (!ret)							\
>> +			goto start;						\
>> +	}									\
>> +} while (0)
>> +
> 
> I'm not sure here either, for pretty much the same reason. As far as
> locking goes, I really think we should prefer something explicit even if
> it means a bit more boilerplate.
> 
> If you still want to push this forward though, this has nothing to do
> with kunit so it should be made a common helper. 

Ack.

> I do think it should be
> done in a separate series though. Ever-expanding series are a nightmare,
> both to contribute and to review :)

Indeed, let me take this separately.

If you agree, I'd prefer to drop EDEADLK handling from the new tests as
well, to allow sorting this out for all in a consistent manner.

Thanks,
Cristian

Re: [PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Maxime Ripard 8 months, 3 weeks ago

On Mon, May 19, 2025 at 01:35:46PM +0300, Cristian Ciocaltea wrote:
> On 5/19/25 10:22 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Fri, Apr 25, 2025 at 01:27:05PM +0300, Cristian Ciocaltea wrote:
> >> In preparation to improve error handling throughout all test cases,
> >> introduce a macro to check for EDEADLK and automate the restart of the
> >> atomic sequence.
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >> index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
> >> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >> @@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
> >>  				test_edid_hdmi_1080p_rgb_max_200mhz);
> >>  }
> >>  
> >> +#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
> >> +	if (ret == -EDEADLK) {							\
> >> +		if (state)							\
> >> +			drm_atomic_state_clear(state);				\
> >> +		ret = drm_modeset_backoff(ctx);					\
> >> +		if (!ret)							\
> >> +			goto start;						\
> >> +	}									\
> >> +} while (0)
> >> +
> > 
> > I'm not sure here either, for pretty much the same reason. As far as
> > locking goes, I really think we should prefer something explicit even if
> > it means a bit more boilerplate.
> > 
> > If you still want to push this forward though, this has nothing to do
> > with kunit so it should be made a common helper. 
> 
> Ack.
> 
> > I do think it should be
> > done in a separate series though. Ever-expanding series are a nightmare,
> > both to contribute and to review :)
> 
> Indeed, let me take this separately.
> 
> If you agree, I'd prefer to drop EDEADLK handling from the new tests as
> well, to allow sorting this out for all in a consistent manner.

We can't unfortunately. Most CI runners now run with WW_DEBUG that will
test for EDEADBLK handling.

Maxime

Re: [PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Cristian Ciocaltea 8 months, 3 weeks ago

On 5/22/25 7:06 PM, Maxime Ripard wrote:
> On Mon, May 19, 2025 at 01:35:46PM +0300, Cristian Ciocaltea wrote:
>> On 5/19/25 10:22 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Apr 25, 2025 at 01:27:05PM +0300, Cristian Ciocaltea wrote:
>>>> In preparation to improve error handling throughout all test cases,
>>>> introduce a macro to check for EDEADLK and automate the restart of the
>>>> atomic sequence.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>>>> index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
>>>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>>>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>>>> @@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
>>>>  				test_edid_hdmi_1080p_rgb_max_200mhz);
>>>>  }
>>>>  
>>>> +#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
>>>> +	if (ret == -EDEADLK) {							\
>>>> +		if (state)							\
>>>> +			drm_atomic_state_clear(state);				\
>>>> +		ret = drm_modeset_backoff(ctx);					\
>>>> +		if (!ret)							\
>>>> +			goto start;						\
>>>> +	}									\
>>>> +} while (0)
>>>> +
>>>
>>> I'm not sure here either, for pretty much the same reason. As far as
>>> locking goes, I really think we should prefer something explicit even if
>>> it means a bit more boilerplate.
>>>
>>> If you still want to push this forward though, this has nothing to do
>>> with kunit so it should be made a common helper. 
>>
>> Ack.
>>
>>> I do think it should be
>>> done in a separate series though. Ever-expanding series are a nightmare,
>>> both to contribute and to review :)
>>
>> Indeed, let me take this separately.
>>
>> If you agree, I'd prefer to drop EDEADLK handling from the new tests as
>> well, to allow sorting this out for all in a consistent manner.
> 
> We can't unfortunately. Most CI runners now run with WW_DEBUG that will
> test for EDEADBLK handling.

Ok, in that case I'll proceed with the explicit error handling for the
new tests only.  And as soon as the series gets merged, I'll come up
with a common helper and apply it for all tests.

Re: [PATCH v4 14/23] drm/tests: hdmi: Add macro to support EDEADLK handling

Posted by Maxime Ripard 8 months, 2 weeks ago

On Thu, May 22, 2025 at 07:47:00PM +0300, Cristian Ciocaltea wrote:
> On 5/22/25 7:06 PM, Maxime Ripard wrote:
> > On Mon, May 19, 2025 at 01:35:46PM +0300, Cristian Ciocaltea wrote:
> >> On 5/19/25 10:22 AM, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 25, 2025 at 01:27:05PM +0300, Cristian Ciocaltea wrote:
> >>>> In preparation to improve error handling throughout all test cases,
> >>>> introduce a macro to check for EDEADLK and automate the restart of the
> >>>> atomic sequence.
> >>>>
> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>> ---
> >>>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> index c8969ee6518954ab4496d3a4398f428bf4104a36..c8bb131d63ea6d0c9e166c8d9ba5e403118cd9f1 100644
> >>>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> @@ -224,6 +224,16 @@ drm_kunit_helper_connector_hdmi_init(struct kunit *test,
> >>>>  				test_edid_hdmi_1080p_rgb_max_200mhz);
> >>>>  }
> >>>>  
> >>>> +#define drm_kunit_atomic_restart_on_deadlock(ret, state, ctx, start) do {	\
> >>>> +	if (ret == -EDEADLK) {							\
> >>>> +		if (state)							\
> >>>> +			drm_atomic_state_clear(state);				\
> >>>> +		ret = drm_modeset_backoff(ctx);					\
> >>>> +		if (!ret)							\
> >>>> +			goto start;						\
> >>>> +	}									\
> >>>> +} while (0)
> >>>> +
> >>>
> >>> I'm not sure here either, for pretty much the same reason. As far as
> >>> locking goes, I really think we should prefer something explicit even if
> >>> it means a bit more boilerplate.
> >>>
> >>> If you still want to push this forward though, this has nothing to do
> >>> with kunit so it should be made a common helper. 
> >>
> >> Ack.
> >>
> >>> I do think it should be
> >>> done in a separate series though. Ever-expanding series are a nightmare,
> >>> both to contribute and to review :)
> >>
> >> Indeed, let me take this separately.
> >>
> >> If you agree, I'd prefer to drop EDEADLK handling from the new tests as
> >> well, to allow sorting this out for all in a consistent manner.
> > 
> > We can't unfortunately. Most CI runners now run with WW_DEBUG that will
> > test for EDEADBLK handling.
> 
> Ok, in that case I'll proceed with the explicit error handling for the
> new tests only.  And as soon as the series gets merged, I'll come up
> with a common helper and apply it for all tests.

Sounds like a good plan :)

Maxime