On Tue, Jul 21, 2026 at 10:36 PM <[email protected]> wrote:
>
> On Wed, 2026-07-15 at 01:14 +0400, Mohamed Ahmed wrote:
> > +static int
> > +nouveau_power_read(struct device *dev, u32 attr, int channel, long
> > *val)
> > +{
> > + struct drm_device *drm_dev = dev_get_drvdata(dev);
> > + struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > + struct nvkm_iccsense *iccsense = nvxx_iccsense(drm);
> > + struct nvkm_device *device = drm->nvkm;
> > + struct nvkm_gsp *gsp = device->gsp;
> > +
> > + if (nvkm_gsp_rusd(gsp))
> > + return nouveau_rusd_power_read(drm_dev, gsp, attr,
> > channel, val);
>
> IMO - let's add braces to this conditional
>
> > + else {
> > + if (!iccsense)
> > + return -EOPNOTSUPP;
> > +
> > + switch (attr) {
> > + case hwmon_power_input:
> > + if (drm_dev->switch_power_state !=
> > DRM_SWITCH_POWER_ON)
> > + return -EINVAL;
> > + *val = nvkm_iccsense_read_all(iccsense);
> > + break;
> > + case hwmon_power_max:
> > + *val = iccsense->power_w_max;
> > + break;
> > + case hwmon_power_crit:
> > + *val = iccsense->power_w_crit;
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > + }
> > +}
> > +
>
> ...
>
> > static int
> > nouveau_temp_write(struct device *dev, u32 attr, int channel, long
> > val)
> > {
> > @@ -667,13 +854,39 @@ nouveau_hwmon_init(struct drm_device *dev)
> > struct nvkm_iccsense *iccsense = nvxx_iccsense(drm);
> > struct nvkm_therm *therm = nvxx_therm(drm);
> > struct nvkm_volt *volt = nvxx_volt(drm);
> > + struct nvkm_device *device = drm->nvkm;
> > + struct nvkm_gsp *gsp = device->gsp;
> > const struct attribute_group *special_groups[N_ATTR_GROUPS];
> > struct nouveau_hwmon *hwmon;
> > struct device *hwmon_dev;
> > + bool rusd_active = false;
> > int ret = 0;
> > int i = 0;
> >
> > - if (!iccsense && !therm && !volt) {
> > + if (nvkm_gsp_rusd(gsp)) {
> > + /* Per-channel visibility is latched below from live
> > RUSD
> > + * section validity, so wait for GSP-RM's first poll
> > of both
> > + * requested groups (thermal, power). It is normally
> > completed
> > + * long before we get here and this is a last
> > resort.
> > + */
> > + unsigned long timeout = jiffies +
> > + msecs_to_jiffies(2 * gsp->rusd.poll_ms);
> > + bool thermal, power;
> > +
> > + for (;;) {
> > + thermal = nouveau_rusd_ok(gsp,
> > NVKM_GSP_RUSD_TEMP_GPU);
> > + power = nouveau_rusd_ok(gsp,
> > NVKM_GSP_RUSD_POWER_GPU) ||
> > + nouveau_rusd_ok(gsp,
> > NVKM_GSP_RUSD_POWER_GPU_AVG) ||
> > + nouveau_rusd_ok(gsp,
> > NVKM_GSP_RUSD_POWER_CAP);
> > + if ((thermal && power) ||
> > !time_before(jiffies, timeout))
> > + break;
> > + msleep(20);
> > + }
>
> Is it possible to use nvif_msec() here?
>
It can work but it's strictly worse than this. nvif_msec() does busy
looping, which would be overkill for this as it would be doing a lot
more reading for something that changes at most once per 500ms poll
cycle, and it keeps the CPU fully occupied with every read. It also
couples us to GPU timing while the main thing we are waiting here on
is whether the GSP's buffer writes are now visible in RAM so GPU side
time doesn't offer us anything. I actually wasn't sure at first how to
properly wait and did go over all the possible timing alternatives and
jiffies looked to be the best here.
That said, to address what Milos mentioned (the weirdness of files
being -ENODATA), I am going to have to move this into nvkm/rusd,
inside r570_rusd_supported()/r570_rusd_read() which doesn't have any
other alternative available to do a similar style waiting loop.
> > +
> > + rusd_active = thermal || power;
> > + }
> > +
> > + if (!iccsense && !therm && !volt && !rusd_active) {
> > NV_DEBUG(drm, "Skipping hwmon registration\n");
> > return 0;
> > }
>
