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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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


> 
>>
>> 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
>>>