Re: [PATCH i-g-t v3] tests/core_setmaster: Adjust loop control in tweak_perm

["Bommu, Krishnaiah" <krishnaiah.bommu@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]


On 31-07-2024 23:26, Emil Velikov wrote:
> Hey everyone,
>
> On Wed, 31 Jul 2024 at 09:31, Bommu Krishnaiah
> <krishnaiah.bommu@intel.com> wrote:
>> The `tweak_perm` function now uses `continue` instead of `break` when
>> encountering non-existent card paths. This change addresses the assumption
>> in existing userspace that card paths are contiguous, which may not be true
>> if cards are dynamically removed.
>>
>> Problem Description:
>> The test fails after running the `core_hotunplug@hot*` subtest, where card0 is missing.
>> This leads to a failure in the subsequent `master-drop-set-user` test.
>>
>> Command sequence for reproducing the issue:
>> - Check available cards: `ls /dev/dri/`
>>    - Output: by-path  card0  renderD128
>> - Run the test: `# ./core_hotunplug --r hotrebind-lateclose`
>> - Check again: ‘ls /dev/dri/’
>>    - Output after failure: by-path  card1  renderD129
>> - Run the test: `# ./core_setmaster --r master-drop-set-user`
>>
>> This failures on both XE and i915 for above sequence.
>>
> The commit message is still a bit strange, but I'll refer that to your
> colleague who already gave you good tips.

tests/core_setmaster: Handle the test even if cards are non-continuous

Although most of userspace assumes drm cards to be populated continously,

this assumption may not be always true. After hot unpluging and 
repluging of card

the card may be non-continous.  To address such scenarios where the 
cards can be non-continous

modify the tweak_perm function to loop all possible 
cardnumbers(card0-card255) to confirm are they populated or not.

Problem Description:

The test fails after running the `core_hotunplug@hot*` subtest, where 
card0 is missing.

This leads to a failure in the subsequent `master-drop-set-user` test.

Command sequence for reproducing the issue:

- Check available cards: `ls /dev/dri/`

- Output: by-path  card0  renderD128

- Run the test: `# ./core_hotunplug --r hotrebind-lateclose`

- Check again: ‘ls /dev/dri/’

- Output after failure: by-path  card1  renderD129

- Run the test: `# ./core_setmaster --r master-drop-set-user`

This failures on both XE and i915 for above sequence.

> Fwiw the "existing usespace" refers to mesa, Xorg (both server and
> drivers) as well as gstreamer, if I recall correctly. There may be
> others assume there's no gaps.
here I am referring to IGT, may be as you said few may be continues, but 
this change works for continues also.
>> Signed-off-by: Bommu Krishnaiah<krishnaiah.bommu@intel.com>
>> Cc: Emil Velikov<emil.l.velikov@gmail.com>
>> Cc: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
>> ---
>>   tests/core_setmaster.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
>> index 9c2083f66..a2758e2f2 100644
>> --- a/tests/core_setmaster.c
>> +++ b/tests/core_setmaster.c
>> @@ -116,9 +116,10 @@ static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
>>          for (i = 0; i < max_perm; i++) {
>>                  snprintf(path, sizeof(path), "/dev/dri/card%u", i);
>>
>> -               /* Existing userspace assumes there's no gaps, do the same. */
>> +               /* Userspace cannot always have continuous card0...N due to
>> +                * dynamic unloading or removal of cards */
> Is this "dynamic unloading or removal of cards" a relatively new
> thing? Back when I wrote there test, unloading DRM modules wasn't
> supported (I believe) hence, it wasn't possible to have gaps.

I don't think dynamic unloading or removal of cards is new

core_hotunplug test verifies unloading of i915/xe driver module

>
> Or does this feature/work that only affects IGT and normal day-to-day
> userspace will never hit this case?

as I mentioned in commit message running sequence is not done before

Regards,

Krishna

>
> Should probably mention a word or two about this in the commit
> message/inline comment.
>
> HTH,
> Emil

[-- Attachment #2: Type: text/html, Size: 42776 bytes --]