Hi Rex,
So amd_vce_state is our hardware representation and
drm_amdgpu_info_vce_clock_table is the IOCTL interface structure? Is
there a reason for not using drm_amdgpu_info_vce_clock_table directly in
the hw manager, except that we don't want to include the IOCTL interface
here?
Cause that would at least be a bit awkward, this is an extra abstraction
layer we usually try to avoid upstream.
I'm fine with this approach for this use case, but when we get more and
more power management exposed to userspace this could raise objections
cause this essentially means we need to duplicate all internal power
management structures for the public interface again.
Regards,
Christian.
Am 12.10.2016 um 11:40 schrieb Zhu, Rex:
>
> Hi Christian,
>
> I mean patch 5 can be changed as
>
> static struct amd_vce_state* pp_dpm_get_vce_clock_table(void *handle,
> int num)
>
> {
>
> PP_CHECK((struct pp_instance *)handle);
>
> hwmgr = ((struct pp_instance *)handle)->hwmgr;
>
> if (hwmgr && num < hwmgr->num_vce_state_tables)
> return &hwmgr->vce_states[i];
>
> Return NULL;
>
> }
>
> And in Patch6.
>
> + case AMDGPU_INFO_VCE_CLOCK_TABLE: {
>
> + struct drm_amdgpu_info_vce_clock_table vce_clk_table = {};
>
> struct amd_vce_state* vce_state;
>
> + for (i = 0; i < AMDGPU_VCE_CLOCK_TABLE_ENTRIES; i++) {
>
> vce_state = amdgpu_dpm_get_vce_clock_table(adev, i);
>
> If (vce_state) {
>
> vce_clk_table.entries[i].sclk =
> vce_state->sclk;
> vce_clk_table.entries[i].mclk = vce_state->mclk;
> vce_clk_table.entries[i].eclk = vce_state->evclk;
>
> } else {
>
> vce_clk_table.num_of_entries = i; //if UMD driver need
> to check number of vce state.
>
> break;
>
> }
>
> }
>
> + return copy_to_user(out, &vce_clk_table,
>
> + min((size_t)size, sizeof(vce_clk_table))) ? -EFAULT : 0;
>
> + }
>
> Best Regards
>
> Rex
>
> *From:*amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] *On
> Behalf Of *Christian König
> *Sent:* Wednesday, October 12, 2016 4:53 PM
> *To:* Zhu, Rex; Alex Deucher; amd-gfx list; Fang, Peter; Zhang, Boyuan
> *Cc:* Deucher, Alexander
> *Subject:* Re: [PATCH 5/6] drm/amdgpu/powerplay: add an implementation
> for get_vce_clock_table
>
> Hi Rex,
>
> the problem with this approach is that you need to copy the entries
> from one structure to another, like we have below:
>
> > + vce_clk_table.entries[i].sclk = hwmgr->vce_states[i].sclk;
> > + vce_clk_table.entries[i].mclk = hwmgr->vce_states[i].mclk;
> > + vce_clk_table.entries[i].eclk = hwmgr->vce_states[i].evclk;
>
> This is not seen as good coding practice and usually isn't allowed
> upstream because it creates two representations in the kernel for the
> same thing.
>
> Regards,
> Christian.
>
> Am 12.10.2016 um 10:34 schrieb Zhu, Rex:
>
> Hi Alex,
>
> In Patch1, I moved struct amdgpu_vce_states to amd_shared.h from
> amdgpu.h. So in powerplay, we used same vce state definition.
>
> So the define of static struct drm_amdgpu_info_vce_clock_table
> pp_dpm_get_vce_clock_table(void *handle)
>
> Can change to
>
> static struct amd_vce_state pp_dpm_get_vce_clock_table(void
> *handle, int num);
>
> and can move those codes to Patch6.
>
> + vce_clk_table.entries[i].sclk
> = hwmgr->vce_states[i].sclk;
>
> + vce_clk_table.entries[i].mclk
> = hwmgr->vce_states[i].mclk;
>
> + vce_clk_table.entries[i].eclk
> = hwmgr->vce_states[i].evclk;
>
> So in powerplay and dpm, we don't need to visit struct
> drm_amdgpu_info_vce_clock_table.
>
> In Patch2, add numer of vce_states in struct dpm. If it is useful
> to UMD driver. We can notify UMD by add a member in struct
> drm_amdgpu_info_vce_clock_table.
>
> Best Regards
>
> Rex
>
> -----Original Message-----
>
> From: Alex Deucher [mailto:alexdeucher at gmail.com]
>
> Sent: Wednesday, October 12, 2016 4:07 AM
>
> To: amd-gfx list; Fang, Peter; Zhang, Boyuan; Zhu, Rex
>
> Cc: Deucher, Alexander
>
> Subject: Re: [PATCH 5/6] drm/amdgpu/powerplay: add an
> implementation for get_vce_clock_table
>
> Rex what is your opinion on exposing the amdgpu drm structure
> through to powerplay? We could do an intermediate interface, but
> that just seems like extra hoops to jump through for pretty
> questionable gain.
>
> Alex
>
> On Fri, Oct 7, 2016 at 2:28 PM, Alex Deucher
> <alexdeucher at gmail.com> <mailto:alexdeucher at gmail.com> wrote:
>
> Used by the powerplay dpm code.
>
> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> <mailto:alexander.deucher at amd.com>
>
> ---
>
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 24
>
> ++++++++++++++++++++++++
>
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>
> index bb8a345..8eee390 100644
>
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>
> @@ -30,6 +30,7 @@
>
> #include "power_state.h"
>
> #include "eventmanager.h"
>
> #include "pp_debug.h"
>
> +#include "drm/amdgpu_drm.h"
>
> #define
> PP_CHECK(handle) \
>
> @@ -821,6 +822,28 @@ static int pp_dpm_read_sensor(void
> *handle, int idx, int32_t *value)
>
> return hwmgr->hwmgr_func->read_sensor(hwmgr, idx,
> value); }
>
> +static struct drm_amdgpu_info_vce_clock_table
>
> +pp_dpm_get_vce_clock_table(void *handle) {
>
> + struct drm_amdgpu_info_vce_clock_table vce_clk_table = {};
>
> + struct pp_hwmgr *hwmgr;
>
> + unsigned i;
>
> +
>
> + if (handle) {
>
> + hwmgr = ((struct pp_instance *)handle)->hwmgr;
>
> +
>
> + if (hwmgr) {
>
> + for (i = 0; i <
> AMDGPU_VCE_CLOCK_TABLE_ENTRIES; i++) {
>
> + vce_clk_table.entries[i].sclk
> = hwmgr->vce_states[i].sclk;
>
> + vce_clk_table.entries[i].mclk
> = hwmgr->vce_states[i].mclk;
>
> + vce_clk_table.entries[i].eclk
> = hwmgr->vce_states[i].evclk;
>
> + }
>
> + }
>
> + }
>
> +
>
> + return vce_clk_table;
>
> +}
>
> +
>
> const struct amd_powerplay_funcs pp_dpm_funcs = {
>
> .get_temperature = pp_dpm_get_temperature,
>
> .load_firmware = pp_dpm_load_fw, @@ -847,6 +870,7 @@
> const
>
> struct amd_powerplay_funcs pp_dpm_funcs = {
>
> .get_mclk_od = pp_dpm_get_mclk_od,
>
> .set_mclk_od = pp_dpm_set_mclk_od,
>
> .read_sensor = pp_dpm_read_sensor,
>
> + .get_vce_clock_table = pp_dpm_get_vce_clock_table,
>
> };
>
> static int amd_pp_instance_init(struct amd_pp_init *pp_init,
>
> --
>
> 2.5.5
>
>
>
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx at lists.freedesktop.org
> <mailto:amd-gfx at lists.freedesktop.org>
>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20161012/418b416d/attachment-0001.html>
