Re: [Nouveau] [PATCH v5 5/5] nouveau_hwmon: Change permissions to numeric
On 26/04/17 19:46, Oscar Salvador wrote:
> This patch replaces the symbolic permissions with the numeric ones,
> and adds me to the authors too.
>
> Signed-off-by: Oscar Salvador
> ---
> drivers/gpu/drm/nouveau/nouveau_hwmon.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 9142779..45b5c85 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -1,5 +1,6 @@
> /*
> - * Copyright 2010 Red Hat Inc.
> + * Copyright 2010 Red Hat Inc. (Ben Skeggs)
Please drop this change.
> + * Copyright 2017 Oscar Salvador
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -19,7 +20,6 @@
> * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> - * Authors: Ben Skeggs
You can't remove people as being the author of something. Just add
yourself if you care about this (I did not care to add my name when I
wrote in this file, because the git history makes more sense nowadays.
Otherwise, I have no strong opinions on this patch. I guess the numeric
representation is easier to read, so I will give you my R-b for this and
let others decide what to do:
Reviewed-by: Martin Peres
> */
>
> #ifdef CONFIG_ACPI
> @@ -56,7 +56,7 @@ nouveau_hwmon_show_temp1_auto_point1_pwm(struct device *d,
> {
> return snprintf(buf, PAGE_SIZE, "%d\n", 100);
> }
> -static SENSOR_DEVICE_ATTR(temp1_auto_point1_pwm, S_IRUGO,
> +static SENSOR_DEVICE_ATTR(temp1_auto_point1_pwm, 0444,
> nouveau_hwmon_show_temp1_auto_point1_pwm, NULL, 0);
>
> static ssize_t
> @@ -88,7 +88,7 @@ nouveau_hwmon_set_temp1_auto_point1_temp(struct device *d,
>
> return count;
> }
> -static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp, S_IRUGO | S_IWUSR,
> +static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp, 0644,
> nouveau_hwmon_temp1_auto_point1_temp,
> nouveau_hwmon_set_temp1_auto_point1_temp, 0);
>
> @@ -121,7 +121,7 @@ nouveau_hwmon_set_temp1_auto_point1_temp_hyst(struct
> device *d,
>
> return count;
> }
> -static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, S_IRUGO | S_IWUSR,
> +static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, 0644,
> nouveau_hwmon_temp1_auto_point1_temp_hyst,
> nouveau_hwmon_set_temp1_auto_point1_temp_hyst, 0);
>
> @@ -255,7 +255,7 @@ nouveau_hwmon_set_pwm1_min(struct device *d, struct
> device_attribute *a,
> return count;
> }
>
> -static SENSOR_DEVICE_ATTR(pwm1_min, S_IRUGO | S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm1_min, 0644,
> nouveau_hwmon_get_pwm1_min,
> nouveau_hwmon_set_pwm1_min, 0);
>
> @@ -295,7 +295,7 @@ nouveau_hwmon_set_pwm1_max(struct device *d, struct
> device_attribute *a,
> return count;
> }
>
> -static SENSOR_DEVICE_ATTR(pwm1_max, S_IRUGO | S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm1_max, 0644,
> nouveau_hwmon_get_pwm1_max,
> nouveau_hwmon_set_pwm1_max, 0);
>
>
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v5 4/5] nouveau_hwmon: Add support for auto_point attributes
The name of the patch is misleading since you also add support for
pwm1_min/max.
Could you add change the title to "nouveau/hwmon: expose the auto_point
and pwm_min/max attrs"? And while you are at it, please change every
commit title to nouveau/hwmon instead of nouveau_hwmon.
On 26/04/17 19:46, Oscar Salvador wrote:
> This patch creates a special group attributes for attrs like "*auto_point*".
> We check if we have support for them, and if we do, we gather them all in
> an attribute_group's structure which is the parameter regarding special groups
> of hwmon_device_register_with_info.
>
> Signed-off-by: Oscar Salvador
> ---
> drivers/gpu/drm/nouveau/nouveau_hwmon.c | 29 -
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 4db65fb..9142779 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -358,6 +358,23 @@ nouveau_hwmon_get_power1_crit(struct nouveau_drm *drm)
> return iccsense->power_w_crit;
> }
>
> +static struct attribute *pwm_fan_sensor_attrs[] = {
> + &sensor_dev_attr_pwm1_min.dev_attr.attr,
> + &sensor_dev_attr_pwm1_max.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(pwm_fan_sensor);
> +
> +static struct attribute *temp1_auto_point_sensor_attrs[] = {
> + &sensor_dev_attr_temp1_auto_point1_pwm.dev_attr.attr,
> + &sensor_dev_attr_temp1_auto_point1_temp.dev_attr.attr,
> + &sensor_dev_attr_temp1_auto_point1_temp_hyst.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(temp1_auto_point_sensor);
> +
> +#define N_ATTR_GROUPS 3
> +
> static const u32 nouveau_config_chip[] = {
> HWMON_C_UPDATE_INTERVAL,
> 0
> @@ -792,17 +809,27 @@ nouveau_hwmon_init(struct drm_device *dev)
> #if defined(CONFIG_HWMON) || (defined(MODULE) &&
> defined(CONFIG_HWMON_MODULE))
> struct nouveau_drm *drm = nouveau_drm(dev);
> struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> + const struct attribute_group *special_groups[N_ATTR_GROUPS];
> struct nouveau_hwmon *hwmon;
> struct device *hwmon_dev;
> int ret = 0;
> + int i = 0;
>
> hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
> if (!hwmon)
> return -ENOMEM;
> hwmon->dev = dev;
>
> + if (therm && therm->attr_get && therm->attr_set) {
> + if (nvkm_therm_temp_get(therm) >= 0)
> + special_groups[i++] = &temp1_auto_point_sensor_group;
> + if (therm->fan_get && therm->fan_get(therm) >= 0)
> + special_groups[i++] = &pwm_fan_sensor_group;
> + }
> +
> + special_groups[i] = 0;
> hwmon_dev = hwmon_device_register_with_info(dev->dev, "nouveau", dev,
> - &nouveau_chip_info, NULL);
> + &nouveau_chip_info, special_groups);
Please align &nouveau_chip_info with dev->dev and split special_groups
on the following line.
> if (IS_ERR(hwmon_dev)) {
> ret = PTR_ERR(hwmon_dev);
> NV_ERROR(drm, "Unable to register hwmon device: %d\n", ret);
>
This commit breaks bisectability, so Ben may have to squash it in the
previous one. Otherwise, this looks good to me:
Reviewed-by: Martin Peres
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v5 3/5] nouveau_hwmon: Remove old code, add .write/.read operations
On 26/04/17 19:46, Oscar Salvador wrote:
> This patch removes old code related to the old api and transforms the
> functions for the new api. It also adds the .write and .read operations.
>
> Signed-off-by: Oscar Salvador
> ---
> drivers/gpu/drm/nouveau/nouveau_hwmon.c | 722
> +++-
> 1 file changed, 249 insertions(+), 473 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index e8ea8d0..4db65fb 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -38,21 +38,17 @@
> #include
>
> #if defined(CONFIG_HWMON) || (defined(MODULE) &&
> defined(CONFIG_HWMON_MODULE))
> -static ssize_t
> -nouveau_hwmon_show_temp(struct device *d, struct device_attribute *a, char
> *buf)
> +static int
> +nouveau_hwmon_show_temp(struct nouveau_drm *drm)
> {
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> int temp = nvkm_therm_temp_get(therm);
>
> if (temp < 0)
> return temp;
>
> - return snprintf(buf, PAGE_SIZE, "%d\n", temp * 1000);
> + return (temp * 1000);
> }
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, nouveau_hwmon_show_temp,
> - NULL, 0);
>
> static ssize_t
> nouveau_hwmon_show_temp1_auto_point1_pwm(struct device *d,
> @@ -129,312 +125,100 @@ static
> SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, S_IRUGO | S_IWUSR,
> nouveau_hwmon_temp1_auto_point1_temp_hyst,
> nouveau_hwmon_set_temp1_auto_point1_temp_hyst, 0);
>
> -static ssize_t
> -nouveau_hwmon_max_temp(struct device *d, struct device_attribute *a, char
> *buf)
> -{
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> - struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n",
> -therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK) * 1000);
> -}
> -static ssize_t
> -nouveau_hwmon_set_max_temp(struct device *d, struct device_attribute *a,
> - const char *buf, size_t count)
> +static int
> +nouveau_hwmon_max_temp(struct nouveau_drm *drm)
> {
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> - long value;
>
> - if (kstrtol(buf, 10, &value) == -EINVAL)
> - return count;
> -
> - therm->attr_set(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK, value / 1000);
> -
> - return count;
> + return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK) * 1000;
> }
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
> nouveau_hwmon_max_temp,
> - nouveau_hwmon_set_max_temp,
> - 0);
>
> -static ssize_t
> -nouveau_hwmon_max_temp_hyst(struct device *d, struct device_attribute *a,
> - char *buf)
> -{
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> - struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n",
> - therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST) * 1000);
> -}
> -static ssize_t
> -nouveau_hwmon_set_max_temp_hyst(struct device *d, struct device_attribute *a,
> - const char *buf, size_t count)
> +static int
> +nouveau_hwmon_max_temp_hyst(struct nouveau_drm *drm)
> {
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> - long value;
> -
> - if (kstrtol(buf, 10, &value) == -EINVAL)
> - return count;
>
> - therm->attr_set(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST,
> - value / 1000);
> -
> - return count;
> + return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST) *
> 1000;
> }
> -static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
> - nouveau_hwmon_max_temp_hyst,
> - nouveau_hwmon_set_max_temp_hyst, 0);
> -
> -static ssize_t
> -nouveau_hwmon_critical_temp(struct device *d, struct device_attribute *a,
> - char *buf)
> -{
> - struct drm_device *dev = dev_get_drvdata(d);
> - struct nouveau_drm *drm = nouveau_drm(dev);
> - struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>
> - return snprintf(buf, PAGE_SIZE, "%d\n",
> -therm->attr_get(therm, NVKM_THERM_ATTR_THRS_CRITICAL) * 1000);
> -}
> -static ssize_t
> -nouveau_hwmon_set_
Re: [Nouveau] [PATCH v5 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string
On 26/04/17 19:46, Oscar Salvador wrote:
> This patch introduces the nouveau_hwmon_ops structure, sets up
> .is_visible and .read_string operations and adds all the functions
> for these operations.
> This is also a preparation for the next patches, where most of the
> work is being done.
> This code doesn't interacture with the old one.
> It's just to make easier the review of all patches.
>
> Signed-off-by: Oscar Salvador
> ---
> drivers/gpu/drm/nouveau/nouveau_hwmon.c | 170
>
> 1 file changed, 170 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 24b40c5..e8ea8d0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -764,6 +764,176 @@ static const struct hwmon_channel_info *nouveau_info[]
> = {
> &nouveau_power,
> NULL
> };
> +
> +static umode_t
> +nouveau_chip_is_visible(const void *data, u32 attr, int channel)
> +{
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
> +{
> + struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> + struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
> +
> + if (!iccsense)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_power_input:
> + if (iccsense->data_valid &&
> + !list_empty(&iccsense->rails))
Not sure if the kernel coding style would mandate !list_empty to be
aligned with the iccsense in the above line, but this is our coding
style at least.
Anyway, this condition has to be moved before the switch as we should
not expose power_max/crit if power_input is not exposed.
> + return 0444;
> + return 0;
> + case hwmon_power_max:
> + if (iccsense->power_w_max)
> + return 0444;
> + return 0;
> + case hwmon_power_crit:
> + if (iccsense->power_w_crit)
> + return 0444;
> + return 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_temp_is_visible(const void *data, u32 attr, int channel)
> +{
> + struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> + struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> + if (therm &&
> + therm->attr_get &&
> + nvkm_therm_temp_get(therm) < 0)
> + return 0;
You can also fold therm->attr_get on the same line as therm.
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + case hwmon_temp_max:
> + case hwmon_temp_max_hyst:
> + case hwmon_temp_crit:
> + case hwmon_temp_crit_hyst:
> + case hwmon_temp_emergency:
> + case hwmon_temp_emergency_hyst:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
> +{
> + struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> + struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> + if (therm &&
> + therm->attr_get &&
> + therm->fan_get &&
> + therm->fan_get(therm) < 0)
> + return 0;
Same, please fold as much as you can in 80 characters ;)
> +
> + switch (attr) {
> + case hwmon_pwm_enable:
> + case hwmon_pwm_input:
> + return 0644;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_input_is_visible(const void *data, u32 attr, int channel)
> +{
> + struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> + struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
> +
> + if (!volt || nvkm_volt_get(volt) < 0)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_in_input:
> + case hwmon_in_label:
> + case hwmon_in_min:
> + case hwmon_in_max:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_fan_is_visible(const void *data, u32 attr, int channel)
> +{
> + struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> + struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> + if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t
> +nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> + int channel)
Could you align 'int channel' with 'const void *data'?
> +{
> + switch
[Nouveau] [Bug 91632] Assert in nouveau_pushbuf_data
https://bugs.freedesktop.org/show_bug.cgi?id=91632 --- Comment #12 from Andrey --- Same here with qupzilla. But Chromium is working fine. -- You are receiving this mail because: You are the assignee for the bug. You are the QA Contact for the bug.___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [Bug 100860] Dual Monitor display problem, after switching screen positions, with 4.10 Kernels
https://bugs.freedesktop.org/show_bug.cgi?id=100860 --- Comment #2 from Ben Skeggs --- Give this commit a try :) https://github.com/skeggsb/nouveau/commit/05f6b91406a382af7d16d6306034c6859afc3eaf -- You are receiving this mail because: You are the assignee for the bug.___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
