Re: [PATCH i-g-t 1/5] lib/power: Allow use of rapl by specifying fd=-1

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 11, 2024 at 07:17:02PM +0200, Kamil Konieczny wrote:
> Hi Ville,
> On 2024-09-16 at 23:18:37 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > igt_power_open() is currently defunct when you don't have
> > the GPU driver loaded, or when you explicitly want to measure
> > via rapl even when using a DGPU. Allow those use cases by
> > accepting fd<0 to indicate that we explicitly want to use rapl.
> 
> 
> This is ok but imho you should document it in function description.

The whole thing is a complete mess already. There should
probably be a completely separate igt_power_gpu_open()
or at the very least  the "gpu" domain special casing
should be handled first, and then fall back to rapl for
everything else.

But I can try to add a small note about the current
behaviour.

> 
> With that
> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  lib/igt_power.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_power.c b/lib/igt_power.c
> > index 810859b134dc..f4d3efcf0cec 100644
> > --- a/lib/igt_power.c
> > +++ b/lib/igt_power.c
> > @@ -106,7 +106,7 @@ int igt_power_open(int fd, struct igt_power *p, const char *domain)
> >  	p->hwmon_fd = -1;
> >  	p->rapl.fd = -1;
> >  
> > -	is_dgfx = is_xe_device(fd) ? xe_has_vram(fd) : gem_has_lmem(fd);
> > +	is_dgfx = fd >= 0 && (is_xe_device(fd) ? xe_has_vram(fd) : gem_has_lmem(fd));
> >  	if (is_dgfx) {
> >  		if (strncmp(domain, "gpu", strlen("gpu")) == 0) {
> >  			p->hwmon_fd = igt_hwmon_open(fd);
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel