Re: [igt-dev] [PATCH] tests/kms_dp_aux_dev: Check DP connection only before tests

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Wed-26-10-2022 08:37 pm, Alex Hung wrote:
> 
> 
> On 2022-10-26 05:14, Modem, Bhanuprakash wrote:
>> On Wed-26-10-2022 01:29 pm, Petri Latvala wrote:
>>> On Tue, Oct 25, 2022 at 08:16:15PM -0600, Alex Hung wrote:
>>>> Without DP port connected, kernel reports "Too many retries"
>>>> that indicates no sink to respond. This is the same across GPUs.
>>>>
>>>> amdgpu ... DM aux hw bus 2: Too many retries, giving up. First 
>>>> error: -5
>>>> i915 ... AUX C/DDI C/PHY C: Too many retries, giving up. First 
>>>> error: -110
>>>>
>>>> In addition, the test can result time out such as below:
>>>>
>>>> /dev/drm_dp_aux0: Connection timed out
>>>> /dev/drm_dp_aux1: Connection timed out
>>
>> These timeouts are already handled in this test: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Figt-gpu-tools%2Ftree%2Ftests%2Fkms_dp_aux_dev.c%23n88&data=05%7C01%7Calex.hung%40amd.com%7Cf247127341e34be42d0608dab7434b53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638023796906119251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=4kt0bcLG6jsPpagnkqwcVWxhvf00Omjr3kjCTs%2BEu2o%3D&reserved=0
>>
>> IMO, It could be fine with existing test, since it'll work as a kind 
>> of negative scenario.
> 1. If a case is known for not going to work, is it necessary to test it?
> 
> 2. copied code here for discussion
> 
> igt_assert(ret == sizeof(buf) ||
>      errno == ETIMEDOUT ||
>      errno == EIO && is_mst_connector(drm_fd, connector_id)));
> 
> As in description, the i915 returns -110 (ETIMEOUT) but amdgpu returns 
> -5 (EIO), and the above code handles i915 well but that's not always 
> true for other scenario.
> 
> Do you prefer I made changes like the below instead?
> 
> igt_assert(ret == sizeof(buf) ||
>      errno == ETIMEDOUT ||
> -    errno == EIO && is_mst_connector(drm_fd, connector_id)));
> +    errno == EIO);

Yes, probably don't break the existing one:

  igt_assert(ret == sizeof(buf) ||
       errno == ETIMEDOUT ||
-     (errno == EIO && is_mst_connector(drm_fd, connector_id)));
+     (errno == EIO &&
+      (is_mst_connector(drm_fd, connector_id || 
is_amdgpu_device(drm_fd)))));

- Bhanu

> 
>>
>> - Bhanu
>>
>>>>
>>>> As a result, checking DP connection alone should be sufficient.
>>>>
>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>> ---
>>>>   tests/kms_dp_aux_dev.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c
>>>> index ed9dd510..7fce74a4 100644
>>>> --- a/tests/kms_dp_aux_dev.c
>>>> +++ b/tests/kms_dp_aux_dev.c
>>>> @@ -53,8 +53,7 @@ static bool test(int drm_fd, uint32_t connector_id)
>>>>       drmModeFreeConnector(connector);
>>>>       igt_assert(dir_fd >= 0);
>>>> -    if (connector->connection != DRM_MODE_CONNECTED &&
>>>> -        is_mst_connector(drm_fd, connector_id)) {
>>>> +    if (connector->connection != DRM_MODE_CONNECTED) {
>>>>           close(dir_fd);
>>>>           return false;
>>>>       }
>>>
>>>
>>> Bhanuprakash, Swati, you wrote/reviewed this piece of code, please 
>>> review
>>> this patch.
>>>
>>>
>>