[i-g-t,v3,1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

Submitted by Kalamarz, Lukasz on Jan. 11, 2019, 4:15 p.m.

Details

Message ID 20190111161548.15732-1-lukasz.kalamarz@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in IGT

Not browsing as part of any series.

Commit Message

Kalamarz, Lukasz Jan. 11, 2019, 4:15 p.m.
In lib code there were few functions using param number instead of
defines. This could lead to some issues (i.e. stolen support
pointing on different parameter) and we would like to avoid them.
v2: Rebased patch

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/ioctl_wrappers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 9f255508..f71f0e32 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -708,12 +708,12 @@  bool gem_mmap__has_wc(int fd)
 		has_wc = 0;
 
 		memset(&gp, 0, sizeof(gp));
-		gp.param = 40; /* MMAP_GTT_VERSION */
+		gp.param = I915_PARAM_MMAP_GTT_VERSION;
 		gp.value = &gtt_version;
 		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 
 		memset(&gp, 0, sizeof(gp));
-		gp.param = 30; /* MMAP_VERSION */
+		gp.param = I915_PARAM_MMAP_VERSION;
 		gp.value = &mmap_version;
 		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 
@@ -964,7 +964,7 @@  static int gem_gtt_type(int fd)
 	int val = 0;
 
 	memset(&gp, 0, sizeof(gp));
-	gp.param = 18; /* HAS_ALIASING_PPGTT */
+	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
 	gp.value = &val;
 
 	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))

Comments

On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> In lib code there were few functions using param number instead of
> defines. This could lead to some issues (i.e. stolen support
> pointing on different parameter) and we would like to avoid them.

The 3 defines you're touching are part of the published kernel uapi, so 
there is no way for them to change and cause issues. The change is still 
worth doing IMO since defines are better than hard-coded numbers.

> v2: Rebased patch
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

All 3 defs are in the i915_drm.h file in IGT, so with a cleaned commit 
message:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

> ---
>   lib/ioctl_wrappers.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..f71f0e32 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
>   		has_wc = 0;
>   
>   		memset(&gp, 0, sizeof(gp));
> -		gp.param = 40; /* MMAP_GTT_VERSION */
> +		gp.param = I915_PARAM_MMAP_GTT_VERSION;
>   		gp.value = &gtt_version;
>   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   
>   		memset(&gp, 0, sizeof(gp));
> -		gp.param = 30; /* MMAP_VERSION */
> +		gp.param = I915_PARAM_MMAP_VERSION;
>   		gp.value = &mmap_version;
>   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   
> @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
>   	int val = 0;
>   
>   	memset(&gp, 0, sizeof(gp));
> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
> +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
>   	gp.value = &val;
>   
>   	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
>
On Fri, 2019-01-11 at 09:54 -0800, Daniele Ceraolo Spurio wrote:
> 

> On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:

> > In lib code there were few functions using param number instead of

> > defines. This could lead to some issues (i.e. stolen support

> > pointing on different parameter) and we would like to avoid them.

> 

> The 3 defines you're touching are part of the published kernel uapi,

> so 

> there is no way for them to change and cause issues. The change is 



In theory what You've said is true, but we had one example within IGT
that may be causing troubles. We had gem_stolen issue, where we are
pointing to param with direct number (38), but then uapi is pointing on
something different (changes in i915 for stole support were never
merged). When running those tests (only hibernate testcase is not run
in during fullrun executions) we may be touching something different
than we expect.

> still 

> worth doing IMO since defines are better than hard-coded numbers.

> 

> > v2: Rebased patch

> > 

> > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>

> > Cc: Michal Winiarski <michal.winiarski@intel.com>

> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>

> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

> > Cc: Chris Wilson <chris@chris-wilson.co.uk>

> 

> All 3 defs are in the i915_drm.h file in IGT, so with a cleaned

> commit 

> message:

> 

> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

> 

> > ---

> >   lib/ioctl_wrappers.c | 6 +++---

> >   1 file changed, 3 insertions(+), 3 deletions(-)

> > 

> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c

> > index 9f255508..f71f0e32 100644

> > --- a/lib/ioctl_wrappers.c

> > +++ b/lib/ioctl_wrappers.c

> > @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)

> >   		has_wc = 0;

> >   

> >   		memset(&gp, 0, sizeof(gp));

> > -		gp.param = 40; /* MMAP_GTT_VERSION */

> > +		gp.param = I915_PARAM_MMAP_GTT_VERSION;

> >   		gp.value = &gtt_version;

> >   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);

> >   

> >   		memset(&gp, 0, sizeof(gp));

> > -		gp.param = 30; /* MMAP_VERSION */

> > +		gp.param = I915_PARAM_MMAP_VERSION;

> >   		gp.value = &mmap_version;

> >   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);

> >   

> > @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)

> >   	int val = 0;

> >   

> >   	memset(&gp, 0, sizeof(gp));

> > -	gp.param = 18; /* HAS_ALIASING_PPGTT */

> > +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;

> >   	gp.value = &val;

> >   

> >   	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))

> >
On 01/14/2019 07:55 AM, Kalamarz, Lukasz wrote:
> On Fri, 2019-01-11 at 09:54 -0800, Daniele Ceraolo Spurio wrote:
>>
>> On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
>>> In lib code there were few functions using param number instead of
>>> defines. This could lead to some issues (i.e. stolen support
>>> pointing on different parameter) and we would like to avoid them.
>>
>> The 3 defines you're touching are part of the published kernel uapi,
>> so
>> there is no way for them to change and cause issues. The change is
> 
> 
> In theory what You've said is true, but we had one example within IGT
> that may be causing troubles. We had gem_stolen issue, where we are
> pointing to param with direct number (38), but then uapi is pointing on
> something different (changes in i915 for stole support were never
> merged). When running those tests (only hibernate testcase is not run
> in during fullrun executions) we may be touching something different
> than we expect.
> 

My comment was regarding exclusively the flags touched by this patch. 
The test in your example was merged to IGT before the flag was merged in 
the kernel, which meant the latter was still not part of the uapi and 
thus could still change.

Daniele

>> still
>> worth doing IMO since defines are better than hard-coded numbers.
>>
>>> v2: Rebased patch
>>>
>>> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> All 3 defs are in the i915_drm.h file in IGT, so with a cleaned
>> commit
>> message:
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>>> ---
>>>    lib/ioctl_wrappers.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>>> index 9f255508..f71f0e32 100644
>>> --- a/lib/ioctl_wrappers.c
>>> +++ b/lib/ioctl_wrappers.c
>>> @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
>>>    		has_wc = 0;
>>>    
>>>    		memset(&gp, 0, sizeof(gp));
>>> -		gp.param = 40; /* MMAP_GTT_VERSION */
>>> +		gp.param = I915_PARAM_MMAP_GTT_VERSION;
>>>    		gp.value = &gtt_version;
>>>    		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>>    
>>>    		memset(&gp, 0, sizeof(gp));
>>> -		gp.param = 30; /* MMAP_VERSION */
>>> +		gp.param = I915_PARAM_MMAP_VERSION;
>>>    		gp.value = &mmap_version;
>>>    		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>>    
>>> @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
>>>    	int val = 0;
>>>    
>>>    	memset(&gp, 0, sizeof(gp));
>>> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
>>> +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
>>>    	gp.value = &val;
>>>    
>>>    	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
>>>