<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> [AMD Official Use Only]<o:p></o:p> <o:p> </o:p> So the final decision is rollback to scnprintf().<o:p></o:p> If we can define our own helper functions like sysfs_emit/sysfs_emit_at <o:p></o:p> but without page boundary aligned limitation to make life easier?<o:p></o:p> <o:p> </o:p> Regards,<o:p></o:p> Lang<o:p></o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Powell, Darren <Darren.Powell@amd.com> Sent: Thursday, September 9, 2021 6:18 AM To: Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings<o:p></o:p> </div> </div> <o:p> </o:p> [AMD Official Use Only]<o:p></o:p> <o:p> </o:p> <div> <div> <o:p> </o:p> </div> <div> <o:p> </o:p> </div> <div class="MsoNormal" align="center" style="text-align:center"> <hr size="2" width="98%" align="center"> </div> <div id="divRplyFwdMsg"> From: Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>> Sent: Wednesday, September 8, 2021 8:43 AM To: Lazar, Lijo <<a href="mailto:Lijo.Lazar@amd.com">Lijo.Lazar@amd.com</a>>; Yu, Lang <<a href="mailto:Lang.Yu@amd.com">Lang.Yu@amd.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>> Cc: Deucher, Alexander <<a href="mailto:Alexander.Deucher@amd.com">Alexander.Deucher@amd.com</a>>; Huang, Ray <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Tian Tao <<a href="mailto:tiantao6@hisilicon.com">tiantao6@hisilicon.com</a>>; Powell, Darren <<a href="mailto:Darren.Powell@amd.com">Darren.Powell@amd.com</a>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings <o:p></o:p> <div> <o:p></o:p> </div> </div> <div> <div> Am 08.09.21 um 12:22 schrieb Lazar, Lijo: > On 9/8/2021 3:08 PM, Christian König wrote: >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo: >>> On 9/8/2021 2:32 PM, Yu, Lang wrote: >>>> [AMD Official Use Only] >>>>> -----Original Message----- >>>>> From: Lazar, Lijo <<a href="mailto:Lijo.Lazar@amd.com">Lijo.Lazar@amd.com</a>> >>>>> Sent: Wednesday, September 8, 2021 4:55 PM >>>>> To: Yu, Lang <<a href="mailto:Lang.Yu@amd.com">Lang.Yu@amd.com</a>>; Christian König >>>>> <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> >>>>> Cc: Deucher, Alexander <<a href="mailto:Alexander.Deucher@amd.com">Alexander.Deucher@amd.com</a>>; Huang, Ray >>>>> <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Tian Tao <<a href="mailto:tiantao6@hisilicon.com">tiantao6@hisilicon.com</a>> >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at >>>>> warnings >>>>> >>>>> >>>>> >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote: >>>>>> [AMD Official Use Only] >>>>>> >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Lazar, Lijo <<a href="mailto:Lijo.Lazar@amd.com">Lijo.Lazar@amd.com</a>> >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM >>>>>>> To: Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>>; Yu, Lang >>>>>>> <<a href="mailto:Lang.Yu@amd.com">Lang.Yu@amd.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> >>>>>>> Cc: Deucher, Alexander <<a href="mailto:Alexander.Deucher@amd.com">Alexander.Deucher@amd.com</a>>; Huang, Ray >>>>>>> <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Tian Tao <<a href="mailto:tiantao6@hisilicon.com">tiantao6@hisilicon.com</a>> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at >>>>>>> warnings >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote: >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu: >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf >>>>>>>>> address. Make them happy! >>>>>>>>> >>>>>>>>> Warning Log: >>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [ >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765 >>>>>>>>> sysfs_emit_at+0x4a/0xa0 >>>>>>>>> [ 492.654805] Call Trace: >>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [ >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [ >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [ >>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713] >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107] >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620] >>>>>>>>> dev_attr_show+0x1d/0x40 >>>>>>>> >>>>>>>> Mhm, that at least partially doesn't looks like the right >>>>>>>> approach to me. >>>>>>>> >>>>>>>> Why do we have string printing and sysfs code in the hardware >>>>>>>> version specific backend in the first place? >>>>>>>> >>>>>>> >>>>>>> This is a callback meant for printing ASIC specific information to >>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is >>>>>>> to the callback API. >>>>>>> >>>>>>>> That stuff needs to be implemented for each hardware generation >>>>>>>> and >>>>>>>> is now cluttered with sysfs buffer offset calculations. >>>>>>>> >>>>>>> >>>>>>> Looks like the warning happened because of this usage. >>>>>>> >>>>>>> size = amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_SCLK, buf); >>>>>>> size += amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_MCLK, >>>>>>> buf+size); >>>>>>> size += amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_VDDC_CURVE, buf+size); >>>>>>> size += amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_VDDGFX_OFFSET, buf+size); >>>>>>> size += amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_RANGE, >>>>>>> buf+size); >>>>>>> size += amdgpu_dpm_print_clock_levels(adev, >>>>>>> OD_CCLK, >>>>>>> buf+size); >>>>>>> >>>>>>> >>>>>> [Yu, Lang] >>>>>> Yes. So it is fine we just fix the caller >>>>>> amdgpu_get_pp_od_clk_voltage like >>>>> following: >>>>>> >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev, >>>>>> struct device_attribute *attr, >>>>>> char *buf) >>>>>> { >>>>>> struct drm_device *ddev = dev_get_drvdata(dev); >>>>>> struct amdgpu_device *adev = drm_to_adev(ddev); >>>>>> ssize_t size, offset; >>>>>> int ret, i; >>>>>> char temp_buf[512]; >>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE, >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK}; >>>>>> >>>>>> if (amdgpu_in_reset(adev)) >>>>>> return -EPERM; >>>>>> if (adev->in_suspend && !adev->in_runpm) >>>>>> return -EPERM; >>>>>> >>>>>> ret = pm_runtime_get_sync(ddev->dev); >>>>>> if (ret < 0) { >>>>>> pm_runtime_put_autosuspend(ddev->dev); >>>>>> return ret; >>>>>> } >>>>>> >>>>>> offset = 0; >>>>>> >>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) { >>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) { >>>>>> size = amdgpu_dpm_print_clock_levels(adev, >>>>> clock_type[i], buf); >>>>>> if (offset + size > PAGE_SIZE) >>>>>> break; >>>>>> memcpy(temp_buf + offset, buf, size); >>>>>> offset += size; >>>>>> } >>>>>> memcpy(buf, temp_buf, offset); >>>>>> size = offset; >>>>>> } else { >>>>>> size = sysfs_emit(buf, "\n"); >>>>>> } >>>>>> pm_runtime_mark_last_busy(ddev->dev); >>>>>> pm_runtime_put_autosuspend(ddev->dev); >>>>>> >>>>>> return size; >>>>>> } >>>>>> >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe >>>>> another arg to >>>>> pass offset along with buf? >>>>> >>>> [Yu, Lang] >>>> Actually, the buf address contains the offset(offset_in_page(buf)) . >>> >>> Though it's not a problem based on codeflow, static analysis tools >>> might complain. >>> >>>> Or we just rollback to sprintf/snprintf. >>>> >>> >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took >>> the effort to convert these, he may have some other ideas. The changes I made were intended to simply replace snprintf with sysfs_emit as per <o:p></o:p> </div> <div> linux/Documentation/filesystems/sysfs.rst:246,247 <o:p></o:p> <div> - show() should only use sysfs_emit() or sysfs_emit_at() when formatting<o:p></o:p> </div> the value to be returned to user space.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> I specifically tried not to change the design, but only as I didn't have <o:p></o:p> </div> <div> background in Power Management.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> >> >> This is not what I meant. See from the design point of view the >> print_clock_levels() callback is the bad idea to begin with. >> >> What we should have instead is a callback which returns the clock as >> a value which is then printed in the amdgpu_get_pp_od_clk_voltage() >> function. >> >> This avoids passing around the buffer and remaining size everywhere >> and also guarantees that the sysfs have unified printing over all >> hardware generations. >> > > The scenario is one node used for multiple parameters - OD_SCLK, > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with > lots of nodes). On top of it, the parameters supported (for ex: CCLK > is not valid on dGPUs), the number of levels supported etc. vary > across ASICs. There has to be multiple calls or the call needs to > return multiple values for a single parameter (for ex: up to 4, 8 or > 16 levels of GFXCLK depending on ASIC). Well exactly that is questionable design for sysfs. See the sysfs_emit() and sysfs_emit_at() functions are designed the way they are because you should have only one value per file, which is then printed at exactly one location. Take a look at the documentation for sysfs for more details. > I don't know the history of the callback, mostly it was considered > more efficient to print it directly rather than fetch and print. > Alex/Evan may know the details. Yeah, somebody with a bit more background in power management needs to take a closer look at this here. Just keep me looped in. Regards, Christian. > > Thanks, > Lijo > >> Regards, >> Christian. >><o:p></o:p> </div> </div> </div> </div> </div> </body> </html>