Re: [PATCH] radeon: Make PM info available to all, not just debug users

["Christian König" <deathsimple@vodafone.de>]

On 02.06.2012 18:08, Lauri Kasanen wrote:
> Hi all
>
> This moves the pm_info file from debugfs to next to the other two power files.
>
> Requested by several users at Phoronix.
>
> PS: Please CC me. Also please be gentle, it's my first step in kernel-land ;)
>
> Signed-off-by: Lauri Kasanen<cand@gmx.com>
Hui? What should this be good for?

Sysfs files are for setting driver parameters, like the power management 
method or profile currently in use. One major advantage of sysfs is the 
strict rules for a permanent and machine usable interface, for example 
it is mandatory to only specify one parameter per sysfs file.

Debugfs on the other hand should be used for human readable 
informations, e.g. the printing the current clocks in a human readable 
form. Also you don't need a debug build or turn on any other debugging 
facility to get those information, just take a look under 
"sys/kernel/debug/dri/*".

So the code is actually quite valid as it is.

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/radeon_pm.c |   86 ++++++++++++++++++-----------------
>   1 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index 0882554..7aab18f 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -45,7 +45,6 @@ static const char *radeon_pm_state_type_name[5] = {
>   };
>
>   static void radeon_dynpm_idle_work_handler(struct work_struct *work);
> -static int radeon_debugfs_pm_init(struct radeon_device *rdev);
>   static bool radeon_pm_in_vbl(struct radeon_device *rdev);
>   static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev, bool finish);
>   static void radeon_pm_update_profile(struct radeon_device *rdev);
> @@ -437,8 +436,49 @@ fail:
>   	return count;
>   }
>
> +static ssize_t radeon_get_pm_info(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
> +	struct radeon_device *rdev = ddev->dev_private;
> +
> +	ssize_t curpos, len = PAGE_SIZE;
> +	char *tmp;
> +
> +	curpos = snprintf(buf, len,
> +			  "default engine clock: %u0 kHz\n"
> +			  "current engine clock: %u0 kHz\n"
> +			  "default memory clock: %u0 kHz\n",
> +			  rdev->pm.default_sclk,
> +			  radeon_get_engine_clock(rdev),
> +			  rdev->pm.default_mclk);
> +	len -= curpos;
> +
> +	if (rdev->asic->get_memory_clock) {
> +		tmp = buf + curpos;
> +		curpos += snprintf(tmp, len, "current memory clock: %u0 kHz\n", radeon_get_memory_clock(rdev));
> +		len = PAGE_SIZE - curpos;
> +	}
> +
> +	if (rdev->pm.current_vddc) {
> +		tmp = buf + curpos;
> +		curpos += snprintf(tmp, len, "voltage: %u mV\n", rdev->pm.current_vddc);
> +		len = PAGE_SIZE - curpos;
> +	}
> +
> +	if (rdev->asic->get_pcie_lanes) {
> +		tmp = buf + curpos;
> +		curpos += snprintf(tmp, len, "PCIE lanes: %d\n", radeon_get_pcie_lanes(rdev));
> +		len = PAGE_SIZE - curpos;
> +	}
> +
> +	return curpos;
> +}
> +
>   static DEVICE_ATTR(power_profile, S_IRUGO | S_IWUSR, radeon_get_pm_profile, radeon_set_pm_profile);
>   static DEVICE_ATTR(power_method, S_IRUGO | S_IWUSR, radeon_get_pm_method, radeon_set_pm_method);
> +static DEVICE_ATTR(power_info, S_IRUGO, radeon_get_pm_info, NULL);
>
>   static ssize_t radeon_hwmon_show_temp(struct device *dev,
>   				      struct device_attribute *attr,
> @@ -639,14 +679,14 @@ int radeon_pm_init(struct radeon_device *rdev)
>   		ret = device_create_file(rdev->dev,&dev_attr_power_method);
>   		if (ret)
>   			DRM_ERROR("failed to create device file for power method\n");
> +		ret = device_create_file(rdev->dev,&dev_attr_power_info);
> +		if (ret)
> +			DRM_ERROR("failed to create device file for power info\n");
>
>   #ifdef CONFIG_ACPI
>   		rdev->acpi_nb.notifier_call = radeon_acpi_event;
>   		register_acpi_notifier(&rdev->acpi_nb);
>   #endif
> -		if (radeon_debugfs_pm_init(rdev)) {
> -			DRM_ERROR("Failed to register debugfs file for PM!\n");
> -		}
>
>   		DRM_INFO("radeon: power management initialized\n");
>   	}
> @@ -843,41 +883,3 @@ static void radeon_dynpm_idle_work_handler(struct work_struct *work)
>   	mutex_unlock(&rdev->pm.mutex);
>   	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
>   }
> -
> -/*
> - * Debugfs info
> - */
> -#if defined(CONFIG_DEBUG_FS)
> -
> -static int radeon_debugfs_pm_info(struct seq_file *m, void *data)
> -{
> -	struct drm_info_node *node = (struct drm_info_node *) m->private;
> -	struct drm_device *dev = node->minor->dev;
> -	struct radeon_device *rdev = dev->dev_private;
> -
> -	seq_printf(m, "default engine clock: %u0 kHz\n", rdev->pm.default_sclk);
> -	seq_printf(m, "current engine clock: %u0 kHz\n", radeon_get_engine_clock(rdev));
> -	seq_printf(m, "default memory clock: %u0 kHz\n", rdev->pm.default_mclk);
> -	if (rdev->asic->pm.get_memory_clock)
> -		seq_printf(m, "current memory clock: %u0 kHz\n", radeon_get_memory_clock(rdev));
> -	if (rdev->pm.current_vddc)
> -		seq_printf(m, "voltage: %u mV\n", rdev->pm.current_vddc);
> -	if (rdev->asic->pm.get_pcie_lanes)
> -		seq_printf(m, "PCIE lanes: %d\n", radeon_get_pcie_lanes(rdev));
> -
> -	return 0;
> -}
> -
> -static struct drm_info_list radeon_pm_info_list[] = {
> -	{"radeon_pm_info", radeon_debugfs_pm_info, 0, NULL},
> -};
> -#endif
> -
> -static int radeon_debugfs_pm_init(struct radeon_device *rdev)
> -{
> -#if defined(CONFIG_DEBUG_FS)
> -	return radeon_debugfs_add_files(rdev, radeon_pm_info_list, ARRAY_SIZE(radeon_pm_info_list));
> -#else
> -	return 0;
> -#endif
> -}