Re: [PATCH v2 i-g-t 2/2] benchmarks/gem_wsim: Fix mmap for discrete graphics cards

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 7/25/2024 11:06 AM, Michal Wajdeczko wrote:
> 
> 
> On 25.07.2024 09:26, Bernatowicz, Marcin wrote:
>> Hi Kamil,
>>
>> On 7/23/2024 12:36 PM, Kamil Konieczny wrote:
>>> Hi Marcin,
>>> On 2023-12-14 at 21:13:02 +0100, Marcin Bernatowicz wrote:
>>>> It appears that 'gem_mmap__wc' doesn't work for discrete graphics cards
>>>> and 'I915_MMAP_OFFSET_FIXED' is needed instead.
>>>> Adopt the mapping approach from 'lib/igt_dummyload'.
>>>>
>>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>>> ---
>>>>    benchmarks/gem_wsim.c | 11 ++++++++++-
>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>>>> index a1db37d4e..eafd9cab8 100644
>>>> --- a/benchmarks/gem_wsim.c
>>>> +++ b/benchmarks/gem_wsim.c
>>>> @@ -1502,7 +1502,16 @@ static unsigned int create_bb(struct w_step
>>>> *w, int self)
>>>>        gem_set_domain(fd, w->bb_handle,
>>>>                   I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC);
>>>>    -    cs = ptr = gem_mmap__wc(fd, w->bb_handle, 0, w->bb_size,
>>>> PROT_WRITE);
>>>> +    if (__gem_set_caching(fd, w->bb_handle,
>>>> +                  I915_CACHING_CACHED) == 0) {
>>>> +        cs = ptr = gem_mmap__cpu(fd, w->bb_handle,
>>>> +                       0, w->bb_size,
>>>> +                       PROT_READ | PROT_WRITE);
>>>> +    } else
>>>
>>> Be consistent, use brace after 'else':
>>>      } else {
>>>
>>>> +        cs = ptr = gem_mmap__device_coherent(fd,
>>>> +                               w->bb_handle,
>>>> +                               0, w->bb_size,
>>>> +                               PROT_READ | PROT_WRITE);
>>>
>>> so it will look like:
>>>      } else {
>>>          cs = ptr = gem_mmap__device_coherent(fd, w->bb_handle, 0,
>>>                               w->bb_size,
>>>                               PROT_READ | PROT_WRITE);
>>>      }
>> ok
>>>
>>> Please use checkpatch.pl script from Linux kernel, it could spot
>>> such problems, see also CONTRIBUTE.md for some helpful options.
>>
>> Strange, I did run checkpatch.pl but it didn't shout:
>>
>> -----------------------------------------------------------------
>> ./0001-benchmarks-gem_wsim-Support-gens-without-relocations.patch
>> -----------------------------------------------------------------
>> total: 0 errors, 0 warnings, 230 lines checked
>>
>> ./0001-benchmarks-gem_wsim-Support-gens-without-relocations.patch has no
>> obvious style problems and is ready for submission.
>> -----------------------------------------------------------------
>> ./0002-benchmarks-gem_wsim-Fix-mmap-for-discrete-graphics-c.patch
>> -----------------------------------------------------------------
>> total: 0 errors, 0 warnings, 17 lines checked
>>
> 
> try with --strict option
> 
> CHECK: multiple assignments should be avoided
> #12: FILE: benchmarks/gem_wsim.c:1507:
> +               cs = ptr = gem_mmap__cpu(fd, w->bb_handle,
> 
> CHECK: Unbalanced braces around else statement
> #15: FILE: benchmarks/gem_wsim.c:1510:
> +       } else
> 
> CHECK: multiple assignments should be avoided
> #16: FILE: benchmarks/gem_wsim.c:1511:
> +               cs = ptr = gem_mmap__device_coherent(fd,
> 
> total: 0 errors, 0 warnings, 3 checks, 17 lines checked
>

Thanks, my bad, I missed the options mentioned in CONTRIBUTE.md

> 
>>
>>>
>>> With above fixed
>>> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>
>> Thanks
>>>
>>>>          /* Store initial 64b timestamp: start */
>>>>        *cs++ = MI_LOAD_REGISTER_IMM(1) | MI_CS_MMIO_DST;
>>>> -- 
>>>> 2.31.1
>>>>