[PATCH i-g-t v2 2/2] tests/intel: check drmModeGetConnector() return in i915_pm_rpm

[Kamil Konieczny]

Hi Luca,
On 2024-05-28 at 10:58:55 +0300, Luca Coelho wrote:

this subject imho is too long:

[PATCH i-g-t v2 2/2] tests/intel: check drmModeGetConnector() return in i915_pm_rpm

It should have test name at begin, not at end:

tests/intel/i915_pm_rpm: Fix bug in init_mode_set_data()

or

tests/intel/i915_pm_rpm: check drmModeGetConnector() before use

or find out something better.

> The drmModeGetConnector() function can return NULL in some cases, so
> we need to check the return value before accessing it.  This is not
> being checked in init_mode_set_data(), so fix that.

Please cite few lines from bug report:

Starting subtest: module-reload
Received signal SIGSEGV.
Stack trace: 
#0 [fatal_sig_handler+0x10f]
#1 [killpg+0x40]
#2 [setup_environment+0x33f]

> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10911
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>

Add here also some KMS devs to Cc, I added Jani, Juha-Pekka and Bhanuprakash.

Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
Cc: Jani Saarinen <jani.saarinen at intel.com>
Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>

> Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> ---
>  tests/intel/i915_pm_rpm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/intel/i915_pm_rpm.c b/tests/intel/i915_pm_rpm.c
> index 0ea5fbd8afd9..3b4034f24a57 100644
> --- a/tests/intel/i915_pm_rpm.c
> +++ b/tests/intel/i915_pm_rpm.c
> @@ -515,6 +515,12 @@ static void init_mode_set_data(struct mode_set_data *data)
>  			data->connectors[i] =
>  				drmModeGetConnector(drm_fd,
>  						    data->res->connectors[i]);
> +			if (!data->connectors[i]) {

imho we should also check it later before using this or edids[]
so maybe this should really be a warn? Or even igt_assert? Or maybe
we should lower number of available connectors? Please ask KMS devs
for possible solution here.

Above code should work if you also fix fini_mode_set_data() below
and also at one more function:

grep -n count_connectors tests/intel/i915_pm_rpm.c

319:    for (int i = 0; i < data->res->count_connectors; i++) {
513:            igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
514:            for (int i = 0; i < data->res->count_connectors; i++) {
531:            for (int i = 0; i < data->res->count_connectors; i++) {
                        

Regards,
Kamil

> +				igt_debug("Could not read connector %u\n",
> +					  data->res->connectors[i]);
> +				continue;
> +			}
> +
>  			data->edids[i] = get_connector_edid(data->connectors[i], i);
>  		}
>  
> -- 
> 2.39.2
>