Re: [PATCH 27/57] drm/amdgpu: Add vega20 soc init sequence on emulator (v3)

[Grazvydas Ignotas]

On Wed, May 16, 2018 at 2:11 PM, Grazvydas Ignotas <nota...@gmail.com> wrote:
> On Tue, May 15, 2018 at 5:59 PM, Alex Deucher <alexdeuc...@gmail.com> wrote:
>> From: Shaoyun Liu <shaoyun@amd.com>
>>
>> v2: cleanups (Alex)
>> v3: make it vega20 only (Alex)
>>
>> Signed-off-by: Shaoyun Liu <shaoyun@amd.com>
>> Acked-by: Alex Deucher <alexander.deuc...@amd.com>
>> Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/emu_soc.c | 10091 
>> +
>>  1 file changed, 10091 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/emu_soc.c 
>> b/drivers/gpu/drm/amd/amdgpu/emu_soc.c
>> index d72c25c1b987..91f00fbe550a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/emu_soc.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/emu_soc.c
>> @@ -26,8 +26,10099 @@
>>  #include "soc15_common.h"
>>  #include "soc15_hw_ip.h"
>>
>> +static void wreg32_idx_byteoffset(struct amdgpu_device *adev, u32 offset, 
>> u32 value) {
>> +
>> +   static u32 maxoffset = 0;
>> +   static int count = 0;
>> +
>> +   WREG32(0xc, offset);
>> +   RREG32(0xc);
>> +   WREG32(0xd, value);
>> +   RREG32(0xd);
>> +
>> +   if (offset > maxoffset)
>> +   maxoffset = offset;
>> +
>> +   count++;
>> +   if (count % 100 == 0) {
>> +   DRM_INFO("%5d registers written, max offset %08x\n", count, 
>> maxoffset);
>> +   msleep(1);
>> +   }
>> +
>> +}
>> +
>> +static void vg20_lsd_soc_init_with_umc(struct amdgpu_device *adev)
>> +{
>> +
>> +   wreg32_idx_byteoffset(adev, 0x10131800, 0x40a40);
>> +   wreg32_idx_byteoffset(adev, 0x10141010, 0x1000);
>> +   wreg32_idx_byteoffset(adev, 0x10134008, 0xfa042021);
>> +   wreg32_idx_byteoffset(adev, 0x387C, 0x3);
>
> Indentation (double tabs).
> Could this be done in a similar way as
> amdgpu_device_program_register_sequence() instead of thousands of
> function calls?

Actually this will bloat the amdgpu module for everyone, while almost
nobody needs this, perhaps hide this under Kconfig option at least?

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 27/57] drm/amdgpu: Add vega20 soc init sequence on emulator (v3)

[Grazvydas Ignotas]

On Tue, May 15, 2018 at 5:59 PM, Alex Deucher  wrote:
> From: Shaoyun Liu 
>
> v2: cleanups (Alex)
> v3: make it vega20 only (Alex)
>
> Signed-off-by: Shaoyun Liu 
> Acked-by: Alex Deucher 
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/emu_soc.c | 10091 
> +
>  1 file changed, 10091 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/emu_soc.c 
> b/drivers/gpu/drm/amd/amdgpu/emu_soc.c
> index d72c25c1b987..91f00fbe550a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/emu_soc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/emu_soc.c
> @@ -26,8 +26,10099 @@
>  #include "soc15_common.h"
>  #include "soc15_hw_ip.h"
>
> +static void wreg32_idx_byteoffset(struct amdgpu_device *adev, u32 offset, 
> u32 value) {
> +
> +   static u32 maxoffset = 0;
> +   static int count = 0;
> +
> +   WREG32(0xc, offset);
> +   RREG32(0xc);
> +   WREG32(0xd, value);
> +   RREG32(0xd);
> +
> +   if (offset > maxoffset)
> +   maxoffset = offset;
> +
> +   count++;
> +   if (count % 100 == 0) {
> +   DRM_INFO("%5d registers written, max offset %08x\n", count, 
> maxoffset);
> +   msleep(1);
> +   }
> +
> +}
> +
> +static void vg20_lsd_soc_init_with_umc(struct amdgpu_device *adev)
> +{
> +
> +   wreg32_idx_byteoffset(adev, 0x10131800, 0x40a40);
> +   wreg32_idx_byteoffset(adev, 0x10141010, 0x1000);
> +   wreg32_idx_byteoffset(adev, 0x10134008, 0xfa042021);
> +   wreg32_idx_byteoffset(adev, 0x387C, 0x3);

Indentation (double tabs).
Could this be done in a similar way as
amdgpu_device_program_register_sequence() instead of thousands of
function calls?

> +
>  int emu_soc_asic_init(struct amdgpu_device *adev)
>  {
> +   if (adev->asic_type == CHIP_VEGA20) {
> +   vg20_lsd_soc_init_with_umc(adev);
> +
> +   amdgpu_device_program_register_sequence(adev,
> +   (const u32 
> *)vega20_golden_init,

Unneeded cast.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 29/57] drm/amdgpu: Add vega20 to dc support check

[Grazvydas Ignotas]

On Tue, May 15, 2018 at 5:59 PM, Alex Deucher  wrote:
> From: Feifei Xu 
>
> Signed-off-by: Feifei Xu 
> Reviewed-by: Alex Deucher 
> Reviewed-by: Hawking Zhang 
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9c9be878aeb5..5aa3b1d69cfe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2170,6 +2170,7 @@ bool amdgpu_device_asic_has_dc_support(enum 
> amd_asic_type asic_type)
> case CHIP_FIJI:
> case CHIP_VEGA10:
> case CHIP_VEGA12:
> +case CHIP_VEGA20:

Inconsistent style (tab vs spaces).

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 30/57] drm/amd: Add dce-12.1 gpio aux registers

[Grazvydas Ignotas]

On Tue, May 15, 2018 at 5:59 PM, Alex Deucher  wrote:
> From: Roman Li 
>
> Updating dce12 register headers by adding dc registers
> required for potential DP LTTPR support.
>
> Signed-off-by: Roman Li 
> Acked-by: Alex Deucher 
> Signed-off-by: Alex Deucher 
> ---
>  .../drm/amd/include/asic_reg/dce/dce_12_0_offset.h |  12 ++
>  .../amd/include/asic_reg/dce/dce_12_0_sh_mask.h| 152 
> +
>  2 files changed, 164 insertions(+)
>  mode change 100644 => 100755 
> drivers/gpu/drm/amd/include/asic_reg/dce/dce_12_0_offset.h
>  mode change 100644 => 100755 
> drivers/gpu/drm/amd/include/asic_reg/dce/dce_12_0_sh_mask.h

Unwanted mode change.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] drm/amd/pp: Add a new pp feature mask bit for OD feature

[Grazvydas Ignotas]

On Tue, Jan 16, 2018 at 2:02 PM, Rex Zhu  wrote:
> when this bit was set on module load,
> driver will allow the user over/under gpu
> clock and voltage through sysfs.
>
> by default, this bit was not set.
>
> Reviewed-by: Alex Deucher 
> Signed-off-by: Rex Zhu 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 +-
>  drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c| 3 +++
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 6 ++
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 7 ++-
>  drivers/gpu/drm/amd/powerplay/inc/hwmgr.h  | 2 ++
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index e679bb8..508a254 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -120,7 +120,7 @@
>  uint amdgpu_sdma_phase_quantum = 32;
>  char *amdgpu_disable_cu = NULL;
>  char *amdgpu_virtual_display = NULL;
> -uint amdgpu_pp_feature_mask = 0x;
> +uint amdgpu_pp_feature_mask = 0x2fff;
>  int amdgpu_ngg = 0;
>  int amdgpu_prim_buf_per_se = 0;
>  int amdgpu_pos_buf_per_se = 0;
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> index e35bdc5..ebfbbcf 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> @@ -935,6 +935,9 @@ int hwmgr_set_user_specify_caps(struct pp_hwmgr *hwmgr)
> PHM_PlatformCaps_CAC);
> }
>
> +   if (hwmgr->feature_mask & PP_OVER_DRIVER_MASK)

PP_OVERDRIVE_MASK? I believe "overdrive" is a single word.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amd/pp: Add custom power profile mode support on Vega10

[Grazvydas Ignotas]

On Wed, Jan 10, 2018 at 1:01 PM, Rex Zhu  wrote:
> Change-Id: I0a554cb6a7a56db63a8fc5af60d5c63f65e021d1
> Signed-off-by: Rex Zhu 
> ---
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c  | 39 +++
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 78 
> ++
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.h |  1 +
>  drivers/gpu/drm/amd/powerplay/inc/hwmgr.h  |  3 +
>  4 files changed, 121 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index 8859b67..3493292 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -1081,6 +1081,43 @@ static int pp_dpm_get_power_profile_state(void *handle,
> return 0;
>  }
>
> +static int pp_get_power_profile_mode(void *handle, char *buf)
> +{
> +   struct pp_hwmgr *hwmgr;
> +   struct pp_instance *pp_handle = (struct pp_instance *)handle;
> +
> +   if (!buf || pp_check(pp_handle))
> +   return -EINVAL;
> +
> +   hwmgr = pp_handle->hwmgr;
> +
> +   if (hwmgr->hwmgr_func->get_power_profile_mode == NULL) {
> +   pr_info("%s was not implemented.\n", __func__);
> +   return snprintf(buf, PAGE_SIZE, "\n");
> +   }
> +
> +   return hwmgr->hwmgr_func->get_power_profile_mode(hwmgr, buf);
> +}
> +
> +static int pp_set_power_profile_mode(void *handle, long *input, uint32_t 
> size)
> +{
> +   struct pp_hwmgr *hwmgr;
> +   struct pp_instance *pp_handle = (struct pp_instance *)handle;
> +
> +

Unnecessary blank line.

> +   if (pp_check(pp_handle))
> +   return -EINVAL;
> +
> +   hwmgr = pp_handle->hwmgr;
> +
> +   if (hwmgr->hwmgr_func->set_power_profile_mode == NULL) {
> +   pr_info("%s was not implemented.\n", __func__);
> +   return -EINVAL;
> +   }
> +
> +   return hwmgr->hwmgr_func->set_power_profile_mode(hwmgr, input, size);
> +}
> +
>  static int pp_dpm_set_power_profile_state(void *handle,
> struct amd_pp_profile *request)
>  {
> @@ -1464,6 +1501,8 @@ static int pp_get_display_mode_validation_clocks(void 
> *handle,
> .switch_power_profile = pp_dpm_switch_power_profile,
> .set_clockgating_by_smu = pp_set_clockgating_by_smu,
> .notify_smu_memory_info = pp_dpm_notify_smu_memory_info,
> +   .get_power_profile_mode = pp_get_power_profile_mode,
> +   .set_power_profile_mode = pp_set_power_profile_mode,
>  /* export to DC */
> .get_sclk = pp_dpm_get_sclk,
> .get_mclk = pp_dpm_get_mclk,
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index 23b7239..e8b6c3d 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -757,6 +757,8 @@ static int vega10_hwmgr_backend_init(struct pp_hwmgr 
> *hwmgr)
>
> hwmgr->backend = data;
>
> +   hwmgr->power_profile_mode = PP_SMC_POWER_PROFILE_VIDEO;
> +
> vega10_set_default_registry_data(hwmgr);
>
> data->disable_dpm_mask = 0xff;
> @@ -3950,6 +3952,7 @@ static int vega10_read_sensor(struct pp_hwmgr *hwmgr, 
> int idx,
> ret = -EINVAL;
> break;
> }
> +
> return ret;
>  }
>
> @@ -5008,6 +5011,79 @@ static int vega10_register_thermal_interrupt(struct 
> pp_hwmgr *hwmgr,
> return 0;
>  }
>
> +static int vega10_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf)
> +{
> +   struct vega10_hwmgr *data = (struct vega10_hwmgr *)(hwmgr->backend);
> +   uint32_t i, size = 0;
> +   uint8_t profile_mode_setting[5][4] = {{70, 60, 1, 3,},

static const, otherwise the compiler has to copy the table to stack on
every call.

> +   {90, 60, 0, 0,},
> +   {70, 60, 0, 0,},
> +   {70, 90, 0, 0,},
> +   {30, 60, 0, 6,},
> +   };
> +   char *profile_name[6] = {"3D_FULL_SCREEN",

static const char * const profile_name[6] = ...

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: use irq-safe lock for kiq->ring_lock

[Grazvydas Ignotas]

On Tue, Nov 7, 2017 at 9:26 AM, Pixel Ding  wrote:
> From: pding 
>
> ...
>
> Signed-off-by: pding 

You need to fix your git config user.name.


Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu/radeon: Use radeon by default for CIK GPUs

[Grazvydas Ignotas]

On Tue, May 30, 2017 at 6:30 AM, Michel Dänzer  wrote:
> On 29/05/17 06:20 PM, Michel Dänzer wrote:
>> From: Michel Dänzer 
>>
>> Even if CONFIG_DRM_AMDGPU_CIK is enabled.
>>
>> There is no feature parity yet for CIK, in particular amdgpu doesn't
>> support HDMI/DisplayPort without DC.
>>
>> Signed-off-by: Michel Dänzer 
>
> [...]
>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
>> b/drivers/gpu/drm/amd/amdgpu/Kconfig
>> index 8d36087fc186..e0121f8b436e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>> @@ -17,11 +17,11 @@ config DRM_AMDGPU_CIK
>>   help
>> Choose this option if you want to enable support for CIK asics.
>>
>> -   CIK is already supported in radeon. If you enable this option,
>> -   support for CIK will be provided by amdgpu and disabled in
>> -   radeon by default. Use module options to override this:
>> +   CIK is already supported in radeon. Support for SI in amdgpu
>
> Consider this "SI" typo fixed in v2.

While you are here, what about adding the full codenames here? You
can't expect every user configuring the kernel to know what SI/CIK
means, it's not even documented in
https://www.x.org/wiki/RadeonFeature/ or wikipedia, while the long
codenames are.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

[Grazvydas Ignotas]

On Sat, May 13, 2017 at 12:50 AM, Li, Samuel <samuel...@amd.com> wrote:
>> If you add this here, you should add the ids file itself and make libdrm 
>> install it too...
> ? Here the ids file is separate from libdrm. It is passed during compilation 
> so that libdrm knows where to get it.

OK then, how will users that compile the OSS stack know where to get
that file (or that they need some new file at all) after your patch is
applied?

>>> +   printf("%s version: %s\n", AMDGPU_ASIC_ID_TABLE,  line);
>>debug leftover?
> This is to print out the ids file version.

Libraries can't print to stdout as it will break all programs that
parse the output, apitrace is one such example.

>
> Thanks,
> Sam
>
> -Original Message-
> From: Grazvydas Ignotas [mailto:nota...@gmail.com]
> Sent: Friday, May 12, 2017 8:16 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
> Subject: Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate 
> file
>
> On Fri, May 12, 2017 at 12:19 AM, Samuel Li <samuel...@amd.com> wrote:
>> From: Xiaojie Yuan <xiaojie.y...@amd.com>
>>
>> v2: fix an off by one error and leading white spaces
>>
>> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
>> Reviewed-by: Junwei Zhang <jerry.zh...@amd.com>
>> Signed-off-by: Samuel Li <samuel...@amd.com>
>> ---
>>  amdgpu/Makefile.am   |   2 +
>>  amdgpu/Makefile.sources  |   2 +-
>>  amdgpu/amdgpu_asic_id.c  | 198
>> +++
>>  amdgpu/amdgpu_asic_id.h  | 165 ---
>>  amdgpu/amdgpu_device.c   |  28 +--
>>  amdgpu/amdgpu_internal.h |  10 +++
>>  6 files changed, 232 insertions(+), 173 deletions(-)  create mode
>> 100644 amdgpu/amdgpu_asic_id.c  delete mode 100644
>> amdgpu/amdgpu_asic_id.h
>>
>> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index
>> cf7bc1b..ecf9e82 100644
>> --- a/amdgpu/Makefile.am
>> +++ b/amdgpu/Makefile.am
>> @@ -30,6 +30,8 @@ AM_CFLAGS = \
>> $(PTHREADSTUBS_CFLAGS) \
>> -I$(top_srcdir)/include/drm
>>
>> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${datadir}/libdrm/amdgpu.ids\"
>
> If you add this here, you should add the ids file itself and make libdrm 
> install it too...
>
>> +
>>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la  libdrm_amdgpu_ladir
>> = $(libdir)  libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0
>> -no-undefined diff --git a/amdgpu/Makefile.sources
>> b/amdgpu/Makefile.sources index 487b9e0..bc3abaa 100644
>> --- a/amdgpu/Makefile.sources
>> +++ b/amdgpu/Makefile.sources
>> @@ -1,5 +1,5 @@
>>  LIBDRM_AMDGPU_FILES := \
>> -   amdgpu_asic_id.h \
>> +   amdgpu_asic_id.c \
>> amdgpu_bo.c \
>> amdgpu_cs.c \
>> amdgpu_device.c \
>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new
>> file mode 100644 index 000..067f38c
>> --- /dev/null
>> +++ b/amdgpu/amdgpu_asic_id.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright © 2017 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> +included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> +SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>> +DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> +OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>> +OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> 

Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

[Grazvydas Ignotas]

On Fri, May 12, 2017 at 12:19 AM, Samuel Li  wrote:
> From: Xiaojie Yuan 
>
> v2: fix an off by one error and leading white spaces
>
> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
> Reviewed-by: Junwei Zhang 
> Signed-off-by: Samuel Li 
> ---
>  amdgpu/Makefile.am   |   2 +
>  amdgpu/Makefile.sources  |   2 +-
>  amdgpu/amdgpu_asic_id.c  | 198 
> +++
>  amdgpu/amdgpu_asic_id.h  | 165 ---
>  amdgpu/amdgpu_device.c   |  28 +--
>  amdgpu/amdgpu_internal.h |  10 +++
>  6 files changed, 232 insertions(+), 173 deletions(-)
>  create mode 100644 amdgpu/amdgpu_asic_id.c
>  delete mode 100644 amdgpu/amdgpu_asic_id.h
>
> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am
> index cf7bc1b..ecf9e82 100644
> --- a/amdgpu/Makefile.am
> +++ b/amdgpu/Makefile.am
> @@ -30,6 +30,8 @@ AM_CFLAGS = \
> $(PTHREADSTUBS_CFLAGS) \
> -I$(top_srcdir)/include/drm
>
> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${datadir}/libdrm/amdgpu.ids\"

If you add this here, you should add the ids file itself and make
libdrm install it too...

> +
>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la
>  libdrm_amdgpu_ladir = $(libdir)
>  libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 -no-undefined
> diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
> index 487b9e0..bc3abaa 100644
> --- a/amdgpu/Makefile.sources
> +++ b/amdgpu/Makefile.sources
> @@ -1,5 +1,5 @@
>  LIBDRM_AMDGPU_FILES := \
> -   amdgpu_asic_id.h \
> +   amdgpu_asic_id.c \
> amdgpu_bo.c \
> amdgpu_cs.c \
> amdgpu_device.c \
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
> new file mode 100644
> index 000..067f38c
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -0,0 +1,198 @@
> +/*
> + * Copyright © 2017 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "amdgpu_drm.h"
> +#include "amdgpu_internal.h"
> +
> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +{
> +   char *buf;
> +   char *s_did;
> +   char *s_rid;
> +   char *s_name;
> +   char *endptr;
> +   int r = 0;
> +
> +   buf = strdup(line);
> +   if (!buf)
> +   return -ENOMEM;
> +
> +   /* ignore empty line and commented line */
> +   if (strlen(line) == 0 || line[0] == '#') {
> +   r = -EAGAIN;
> +   goto out;
> +   }
> +
> +   /* device id */
> +   s_did = strtok(buf, ",");

You can't use strtok() in a library. Any other thread may call
strtok() anytime too and screw you up.

> +   if (!s_did) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +   id->did = strtol(s_did, , 16);
> +   if (*endptr) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +   /* revision id */
> +   s_rid = strtok(NULL, ",");
> +   if (!s_rid) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +   id->rid = strtol(s_rid, , 16);
> +   if (*endptr) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +   /* marketing name */
> +   s_name = strtok(NULL, ",");
> +   if (!s_name) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +   id->marketing_name = strdup(s_name);
> +   if (id->marketing_name == NULL) {
> +   r = -EINVAL;
> +   goto out;
> +   }
> +
> +out:
> +   free(buf);
> +
> +   return r;
> +}
> +
> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> 

Re: [PATCH 3/3] drm/amdgpu: fix to clear ASIC INIT COMPLETE bit on resuming phase

[Grazvydas Ignotas]

On Mon, Apr 10, 2017 at 12:37 PM, Huang Rui  wrote:

> ASIC_INIT_COMPLETE bit must be cleared during S3 resuming phase,
> because VBIOS will check the bit to decide if execute ASIC_Init
> posting via kernel driver.
>
> Signed-off-by: Huang Rui 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 5 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c | 6 +-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> index ad43299..b0dd72a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -1727,8 +1727,11 @@ void amdgpu_atombios_scratch_regs_restore(struct
> amdgpu_device *adev)
>  {
> int i;
>
> -   for (i = 0; i < AMDGPU_BIOS_NUM_SCRATCH; i++)
> +   for (i = 0; i < AMDGPU_BIOS_NUM_SCRATCH; i++) {
> +   if (i == 7)
> +   adev->bios_scratch[i] &=
> ~ATOM_S7_ASIC_INIT_COMPLETE_MASK;
>

Maybe move this line before the loop?
A comment may also be useful so that somebody doesn't delete the code again
in future.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: allow shifts >= 32 in AMDGPU_TILING_SET/GET

[Grazvydas Ignotas]

On Tue, Mar 21, 2017 at 9:44 PM, Marek Olšák  wrote:
> From: Marek Olšák 
>
> also adjust the comments
>
> Signed-off-by: Marek Olšák 
> ---
>  include/uapi/drm/amdgpu_drm.h | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 7c6cc11..7fb9d10 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -204,46 +204,48 @@ union drm_amdgpu_ctx {
>
>  struct drm_amdgpu_gem_userptr {
> __u64   addr;
> __u64   size;
> /* AMDGPU_GEM_USERPTR_* */
> __u32   flags;
> /* Resulting GEM handle */
> __u32   handle;
>  };
>
> +/* SI-CI-VI: */
>  /* same meaning as the GB_TILE_MODE and GL_MACRO_TILE_MODE fields */
>  #define AMDGPU_TILING_ARRAY_MODE_SHIFT 0
>  #define AMDGPU_TILING_ARRAY_MODE_MASK  0xf
>  #define AMDGPU_TILING_PIPE_CONFIG_SHIFT4
>  #define AMDGPU_TILING_PIPE_CONFIG_MASK 0x1f
>  #define AMDGPU_TILING_TILE_SPLIT_SHIFT 9
>  #define AMDGPU_TILING_TILE_SPLIT_MASK  0x7
>  #define AMDGPU_TILING_MICRO_TILE_MODE_SHIFT12
>  #define AMDGPU_TILING_MICRO_TILE_MODE_MASK 0x7
>  #define AMDGPU_TILING_BANK_WIDTH_SHIFT 15
>  #define AMDGPU_TILING_BANK_WIDTH_MASK  0x3
>  #define AMDGPU_TILING_BANK_HEIGHT_SHIFT17
>  #define AMDGPU_TILING_BANK_HEIGHT_MASK 0x3
>  #define AMDGPU_TILING_MACRO_TILE_ASPECT_SHIFT  19
>  #define AMDGPU_TILING_MACRO_TILE_ASPECT_MASK   0x3
>  #define AMDGPU_TILING_NUM_BANKS_SHIFT  21
>  #define AMDGPU_TILING_NUM_BANKS_MASK   0x3
> -/* Tiling flags for GFX9. */
> +
> +/* GFX9 and later: */
>  #define AMDGPU_TILING_SWIZZLE_MODE_SHIFT   0
>  #define AMDGPU_TILING_SWIZZLE_MODE_MASK0x1f
>
>  /* Set/Get helpers for tiling flags. */
>  #define AMDGPU_TILING_SET(field, value) \
> -   (((value) & AMDGPU_TILING_##field##_MASK) << 
> AMDGPU_TILING_##field##_SHIFT)
> +   (((uint64_t)(value) & AMDGPU_TILING_##field##_MASK) << 
> AMDGPU_TILING_##field##_SHIFT)
>  #define AMDGPU_TILING_GET(value, field) \
> -   (((value) >> AMDGPU_TILING_##field##_SHIFT) & 
> AMDGPU_TILING_##field##_MASK)
> +   (((uint64_t)(value) >> AMDGPU_TILING_##field##_SHIFT) & 
> AMDGPU_TILING_##field##_MASK)

Shouldn't it be __u64 instead of uint64_t? The kernel header doesn't
include stdint.h or use any uint* types.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 2/3] amdgpu: vamgr_32 can be a struct instead of a pointer

[Grazvydas Ignotas]

From: Alex Xie <alexbin@amd.com>

vamgr_32 is an integral part of amdgpu_device. We don't need to calloc and free 
it.
This can save CPU time, reduce heap fragmentation.

Signed-off-by: Alex Xie <alexbin@amd.com>
Reviewed-by: Christian König <christian.koe...@amd.com>
[Grazvydas Ignotas: rebase, correct a typo in commit message]
---
 amdgpu/amdgpu_device.c   | 8 ++--
 amdgpu/amdgpu_internal.h | 2 +-
 amdgpu/amdgpu_vamgr.c| 4 ++--
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index cad7133..11714e4 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -131,8 +131,7 @@ static int amdgpu_get_auth(int fd, int *auth)
 
 static void amdgpu_device_free_internal(amdgpu_device_handle dev)
 {
-   amdgpu_vamgr_deinit(dev->vamgr_32);
-   free(dev->vamgr_32);
+   amdgpu_vamgr_deinit(>vamgr_32);
amdgpu_vamgr_deinit(dev->vamgr);
free(dev->vamgr);
util_hash_table_destroy(dev->bo_flink_names);
@@ -270,10 +269,7 @@ int amdgpu_device_initialize(int fd,
if (start > 0x)
goto free_va; /* shouldn't get here */
 
-   dev->vamgr_32 =  calloc(1, sizeof(struct amdgpu_bo_va_mgr));
-   if (dev->vamgr_32 == NULL)
-   goto free_va;
-   amdgpu_vamgr_init(dev->vamgr_32, start, max,
+   amdgpu_vamgr_init(>vamgr_32, start, max,
  dev->dev_info.virtual_address_alignment);
 
*major_version = dev->major_version;
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index 4f039b6..7e237ac 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -87,7 +87,7 @@ struct amdgpu_device {
/** The global VA manager for the whole virtual address space */
struct amdgpu_bo_va_mgr *vamgr;
/** The VA manager for the 32bit address space */
-   struct amdgpu_bo_va_mgr *vamgr_32;
+   struct amdgpu_bo_va_mgr vamgr_32;
 };
 
 struct amdgpu_bo {
diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 8a707cb..4dc4253 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -236,7 +236,7 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
struct amdgpu_bo_va_mgr *vamgr;
 
if (flags & AMDGPU_VA_RANGE_32_BIT)
-   vamgr = dev->vamgr_32;
+   vamgr = >vamgr_32;
else
vamgr = dev->vamgr;
 
@@ -249,7 +249,7 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
if (!(flags & AMDGPU_VA_RANGE_32_BIT) &&
(*va_base_allocated == AMDGPU_INVALID_VA_ADDRESS)) {
/* fallback to 32bit address */
-   vamgr = dev->vamgr_32;
+   vamgr = >vamgr_32;
*va_base_allocated = amdgpu_vamgr_find_va(vamgr, size,
va_base_alignment, va_base_required);
}
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 3/3] amdgpu: vamgr can be a struct instead of a pointer

[Grazvydas Ignotas]

From: Alex Xie <alexbin@amd.com>

vamgr is an integral part of amdgpu_device. We don't need to calloc and free it.
This can save CPU time, reduce heap fragmentation.

Signed-off-by: Alex Xie <alexbin@amd.com>
Reviewed-by: Christian König <christian.koe...@amd.com>
[Grazvydas Ignotas: rebase, correct a typo in commit message]
---
 amdgpu/amdgpu_device.c   | 16 +---
 amdgpu/amdgpu_internal.h |  2 +-
 amdgpu/amdgpu_vamgr.c|  2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index 11714e4..f473d2d 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -132,8 +132,7 @@ static int amdgpu_get_auth(int fd, int *auth)
 static void amdgpu_device_free_internal(amdgpu_device_handle dev)
 {
amdgpu_vamgr_deinit(>vamgr_32);
-   amdgpu_vamgr_deinit(dev->vamgr);
-   free(dev->vamgr);
+   amdgpu_vamgr_deinit(>vamgr);
util_hash_table_destroy(dev->bo_flink_names);
util_hash_table_destroy(dev->bo_handles);
pthread_mutex_destroy(>bo_table_mutex);
@@ -254,16 +253,12 @@ int amdgpu_device_initialize(int fd,
if (r)
goto cleanup;
 
-   dev->vamgr = calloc(1, sizeof(struct amdgpu_bo_va_mgr));
-   if (dev->vamgr == NULL)
-   goto cleanup;
-
-   amdgpu_vamgr_init(dev->vamgr, dev->dev_info.virtual_address_offset,
+   amdgpu_vamgr_init(>vamgr, dev->dev_info.virtual_address_offset,
  dev->dev_info.virtual_address_max,
  dev->dev_info.virtual_address_alignment);
 
max = MIN2(dev->dev_info.virtual_address_max, 0x);
-   start = amdgpu_vamgr_find_va(dev->vamgr,
+   start = amdgpu_vamgr_find_va(>vamgr,
 max - dev->dev_info.virtual_address_offset,
 dev->dev_info.virtual_address_alignment, 
0);
if (start > 0x)
@@ -282,10 +277,9 @@ int amdgpu_device_initialize(int fd,
 
 free_va:
r = -ENOMEM;
-   amdgpu_vamgr_free_va(dev->vamgr, start,
+   amdgpu_vamgr_free_va(>vamgr, start,
 max - dev->dev_info.virtual_address_offset);
-   amdgpu_vamgr_deinit(dev->vamgr);
-   free(dev->vamgr);
+   amdgpu_vamgr_deinit(>vamgr);
 
 cleanup:
if (dev->fd >= 0)
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index 7e237ac..cf119a5 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -85,7 +85,7 @@ struct amdgpu_device {
struct drm_amdgpu_info_device dev_info;
struct amdgpu_gpu_info info;
/** The global VA manager for the whole virtual address space */
-   struct amdgpu_bo_va_mgr *vamgr;
+   struct amdgpu_bo_va_mgr vamgr;
/** The VA manager for the 32bit address space */
struct amdgpu_bo_va_mgr vamgr_32;
 };
diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 4dc4253..2b1388e 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -238,7 +238,7 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
if (flags & AMDGPU_VA_RANGE_32_BIT)
vamgr = >vamgr_32;
else
-   vamgr = dev->vamgr;
+   vamgr = >vamgr;
 
va_base_alignment = MAX2(va_base_alignment, vamgr->va_alignment);
size = ALIGN(size, vamgr->va_alignment);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 2/3] amdgpu: vamgr_32 can be a struct instead of a pointer

[Grazvydas Ignotas]

From: Alex Xie <alexbin@amd.com>

vamgr_32 is an integral part of amdgpu_device. We don't need to calloc and free 
it.
This can save CPU time, reduce heap fragmentation.

Signed-off-by: Alex Xie <alexbin@amd.com>
Reviewed-by: Christian König <christian.koe...@amd.com>
[Grazvydas Ignotas: rebase, correct a typo in commit message]
---
 amdgpu/amdgpu_device.c   | 8 ++--
 amdgpu/amdgpu_internal.h | 2 +-
 amdgpu/amdgpu_vamgr.c| 4 ++--
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index cad7133..11714e4 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -131,8 +131,7 @@ static int amdgpu_get_auth(int fd, int *auth)
 
 static void amdgpu_device_free_internal(amdgpu_device_handle dev)
 {
-   amdgpu_vamgr_deinit(dev->vamgr_32);
-   free(dev->vamgr_32);
+   amdgpu_vamgr_deinit(>vamgr_32);
amdgpu_vamgr_deinit(dev->vamgr);
free(dev->vamgr);
util_hash_table_destroy(dev->bo_flink_names);
@@ -270,10 +269,7 @@ int amdgpu_device_initialize(int fd,
if (start > 0x)
goto free_va; /* shouldn't get here */
 
-   dev->vamgr_32 =  calloc(1, sizeof(struct amdgpu_bo_va_mgr));
-   if (dev->vamgr_32 == NULL)
-   goto free_va;
-   amdgpu_vamgr_init(dev->vamgr_32, start, max,
+   amdgpu_vamgr_init(>vamgr_32, start, max,
  dev->dev_info.virtual_address_alignment);
 
*major_version = dev->major_version;
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index 4f039b6..7e237ac 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -87,7 +87,7 @@ struct amdgpu_device {
/** The global VA manager for the whole virtual address space */
struct amdgpu_bo_va_mgr *vamgr;
/** The VA manager for the 32bit address space */
-   struct amdgpu_bo_va_mgr *vamgr_32;
+   struct amdgpu_bo_va_mgr vamgr_32;
 };
 
 struct amdgpu_bo {
diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 8a707cb..4dc4253 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -236,7 +236,7 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
struct amdgpu_bo_va_mgr *vamgr;
 
if (flags & AMDGPU_VA_RANGE_32_BIT)
-   vamgr = dev->vamgr_32;
+   vamgr = >vamgr_32;
else
vamgr = dev->vamgr;
 
@@ -249,7 +249,7 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
if (!(flags & AMDGPU_VA_RANGE_32_BIT) &&
(*va_base_allocated == AMDGPU_INVALID_VA_ADDRESS)) {
/* fallback to 32bit address */
-   vamgr = dev->vamgr_32;
+   vamgr = >vamgr_32;
*va_base_allocated = amdgpu_vamgr_find_va(vamgr, size,
va_base_alignment, va_base_required);
}
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 1/3] amdgpu: Free/uninit vamgr_32 in theoretically correct order

[Grazvydas Ignotas]

From: Alex Xie 

vamgr_32 is a region inside general VAM range. It is better to free and
deinitialize it before general VAM range.

Signed-off-by: Alex Xie 
Reviewed-by: Christian König 
---
 amdgpu/amdgpu_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index f4ede03..cad7133 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -131,10 +131,10 @@ static int amdgpu_get_auth(int fd, int *auth)
 
 static void amdgpu_device_free_internal(amdgpu_device_handle dev)
 {
-   amdgpu_vamgr_deinit(dev->vamgr);
-   free(dev->vamgr);
amdgpu_vamgr_deinit(dev->vamgr_32);
free(dev->vamgr_32);
+   amdgpu_vamgr_deinit(dev->vamgr);
+   free(dev->vamgr);
util_hash_table_destroy(dev->bo_flink_names);
util_hash_table_destroy(dev->bo_handles);
pthread_mutex_destroy(>bo_table_mutex);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 0/3] some -pro patches for integration

[Grazvydas Ignotas]

I've taken several patches from amdgpu-pro libdrm that look useful
to me and I think can be applied already. The only things I did was
rebasing, fixing some typos and dropping Change-Id.

Alex Xie (3):
  amdgpu: Free/uninit vamgr_32 in theoretically correct order
  amdgpu: vamgr can be a struct instead of a pointer
  amdgpu: vamgr_32 can be a struct instead of a pointer

 amdgpu/amdgpu_device.c   | 24 +++-
 amdgpu/amdgpu_internal.h |  4 ++--
 amdgpu/amdgpu_vamgr.c|  6 +++---
 3 files changed, 12 insertions(+), 22 deletions(-)

-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:guarantee 128dws between vm flush and IB(v3)

[Grazvydas Ignotas]

On Thu, Jan 19, 2017 at 4:32 PM, Christian König
<deathsim...@vodafone.de> wrote:
> Am 19.01.2017 um 14:51 schrieb Grazvydas Ignotas:
>>
>> On Thu, Jan 19, 2017 at 11:10 AM, Christian König
>> <deathsim...@vodafone.de> wrote:
>>>
>>> Am 18.01.2017 um 12:42 schrieb Monk Liu:
>>>>
>>>> @@ -6743,6 +6741,15 @@ static void gfx_v8_ring_emit_cntxcntl(struct
>>>> amdgpu_ring *ring, uint32_t flags)
>>>>  if (amdgpu_sriov_vf(ring->adev))
>>>>  gfx_v8_0_ring_emit_de_meta_init(ring,
>>>>  (flags & AMDGPU_VM_DOMAIN) ? AMDGPU_CSA_VADDR :
>>>> ring->adev->virt.csa_vmid0_addr);
>>>> +
>>>> +   /* We need to pad some NOPs before emit_ib to prevent CE run
>>>> ahead
>>>> of
>>>> +* vm_flush, which may trigger VM fault. */
>>>> +   if (ring->wptr > ring->last_vm_flush_pos) /* no wptr wrapping to
>>>> RB head */
>>>> +   amdgpu_ring_insert_nop(ring, 128 - (ring->wptr -
>>>> ring->last_vm_flush_pos));
>>>
>>>
>>> This can easily result in a negative number, couldn't it?
>>>
>>>> +   else
>>>> +   if (ring->ptr_mask + 1 - ring->last_vm_flush_pos +
>>>> ring->wptr < 128)
>>>> +   amdgpu_ring_insert_nop(ring,
>>>> +   128 - (ring->ptr_mask + 1 -
>>>> ring->last_vm_flush_pos + ring->wptr));
>>>
>>>
>>> I think it would be cleaner if you calculate the number of NOPs needed
>>> first
>>> for both cases and then check if the number isn't negative for both
>>> cases.
>>
>> What about this:
>> 128 - ((ring->wptr - ring->last_vm_flush_pos) & 127)
>
>
> That won't handle the case for negative nop count correctly either.
>
> See when we already emitted more than 128 dw we don't want to add some more.

Let me try again then:

count_added = (ring->wptr - ring->last_vm_flush_pos) & ring->ptr_mask;
if (count_added < 128)
amdgpu_ring_insert_nop(ring, 128 - count_added);

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:guarantee 128dws between vm flush and IB(v3)

[Grazvydas Ignotas]

On Thu, Jan 19, 2017 at 11:10 AM, Christian König
 wrote:
> Am 18.01.2017 um 12:42 schrieb Monk Liu:
>> @@ -6743,6 +6741,15 @@ static void gfx_v8_ring_emit_cntxcntl(struct
>> amdgpu_ring *ring, uint32_t flags)
>> if (amdgpu_sriov_vf(ring->adev))
>> gfx_v8_0_ring_emit_de_meta_init(ring,
>> (flags & AMDGPU_VM_DOMAIN) ? AMDGPU_CSA_VADDR :
>> ring->adev->virt.csa_vmid0_addr);
>> +
>> +   /* We need to pad some NOPs before emit_ib to prevent CE run ahead
>> of
>> +* vm_flush, which may trigger VM fault. */
>> +   if (ring->wptr > ring->last_vm_flush_pos) /* no wptr wrapping to
>> RB head */
>> +   amdgpu_ring_insert_nop(ring, 128 - (ring->wptr -
>> ring->last_vm_flush_pos));
>
>
> This can easily result in a negative number, couldn't it?
>
>> +   else
>> +   if (ring->ptr_mask + 1 - ring->last_vm_flush_pos +
>> ring->wptr < 128)
>> +   amdgpu_ring_insert_nop(ring,
>> +   128 - (ring->ptr_mask + 1 -
>> ring->last_vm_flush_pos + ring->wptr));
>
>
> I think it would be cleaner if you calculate the number of NOPs needed first
> for both cases and then check if the number isn't negative for both cases.

What about this:
128 - ((ring->wptr - ring->last_vm_flush_pos) & 127)

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)

[Grazvydas Ignotas]

On Thu, Dec 15, 2016 at 4:12 PM, Christian König
 wrote:
>
> Regarding which error code to return I think that Emil has the right idea
> here.
>
> Returning -EINVAL usually means that userspace provided an invalid value,
> but in this case it doesn't matter which value the UMD provide all of them
> would be invalid because starting with Polaris the hardware/firmware simply
> doesn't work this way any more.
>
> So using -ENODEV or maybe -ENODATA indeed sound like the right think to do
> here.

What about ERANGE then, "Math result not representable" aka infinity?
To me ENODEV is more like "the GPU you are asking about is not there"
and ENODATA "information you ask for is not known", although the later
still somewhat makes sense.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)

[Grazvydas Ignotas]

On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam  wrote:
>>-Original Message-
>>From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>>Sent: Thursday, December 15, 2016 5:01 PM
>>To: Nath, Arindam
>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>Koenig, Christian
>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>handles (v3)
>>
>>On 15 December 2016 at 07:30, Nath, Arindam 
>>wrote:
-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
Sent: Wednesday, December 14, 2016 9:26 PM
To: Nath, Arindam
Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
Koenig, Christian
Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
handles (v3)

On 12 December 2016 at 18:49,   wrote:
> From: Arindam Nath 
>
> Change History
> --
>
> v3: changes suggested by Christian
> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
>   query type.
> - Add a check for asic_type to be less than
>   CHIP_POLARIS10 since starting Polaris, we support
>   unlimited UVD instances.
> - Add kerneldoc style comment for
>   amdgpu_uvd_used_handles().
>
> v2: as suggested by Christian
> - Add a new query AMDGPU_INFO_NUM_HANDLES
> - Create a helper function to return the number
>   of currently used UVD handles.
> - Modify the logic to count the number of used
>   UVD handles since handles can be freed in
>   non-linear fashion.
>
> v1:
> - User might want to query the maximum number of UVD
>   instances supported by firmware. In addition to that,
>   if there are multiple applications using UVD handles
>   at the same time, he might also want to query the
>   currently used number of handles.
>
>   For this we add two variables max_handles and
>   used_handles inside drm_amdgpu_info_hw_ip. So now
>   an application (or libdrm) can use AMDGPU_INFO IOCTL
>   with AMDGPU_INFO_HW_IP_INFO query type to get these
>   values.
>
> Signed-off-by: Arindam Nath 
> Reviewed-by: Christian König 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
+
>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
+
>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h |  1 +
>  include/uapi/drm/amdgpu_drm.h   |  9 +
>  4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 174eb59..3273d8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
*dev, void *data, struct drm_file
> return -EINVAL;
> }
> }
> +   case AMDGPU_INFO_NUM_HANDLES: {
> +   struct drm_amdgpu_info_num_handles handle;
> +
> +   switch (info->query_hw_ip.type) {
> +   case AMDGPU_HW_IP_UVD:
> +   /* Starting Polaris, we support unlimited UVD 
> handles */
> +   if (adev->asic_type < CHIP_POLARIS10) {
> +   handle.uvd_max_handles = 
> adev->uvd.max_handles;
> +   handle.uvd_used_handles =
amdgpu_uvd_used_handles(adev);
> +
> +   return copy_to_user(out, ,
> +   min((size_t)size, 
> sizeof(handle))) ? -EFAULT : 0;
> +   } else {
> +   return -EINVAL;
Using EINVAL doesn't seem right here. As per man 3 ioctl

  EINVAL The request or arg argument is not valid for this device.

A bit further down you can see the one you want.

  ENODEV The fildes argument refers to a valid STREAMS device, but
the corresponding device driver does not support the ioctl() function.
>>>
>>> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
>>our case ioctl() is supported.
>>>
>>> Since we extract the query type from arg passed to ioctl(), and it is this
>>query AMDGPU_INFO_NUM_HANDLES which is not supported by driver (for
>>> Polaris), doesn’t returning EINVAL make more sense here?
>>>
>>Unless I'm reading the code incorrectly - CHIP_POLARIS10 and older do
>>not have support the query. Thus ENODEV is the one you want to use.
>>Furthermore EINVAL is (mostly) used to indicate incorrect input
>>(failed input validation) which userspace uses to check if kernel is
>>too old/does not support X.
>
> Actually, the code says only asics older than 

Re: [PATCH 1/7] drm/amdgpu/powerplay: pp module only enable smu when dpm disabled.

[Grazvydas Ignotas]

On Wed, Nov 2, 2016 at 12:27 PM, Rex Zhu  wrote:
> Change-Id: I3288a5a4bbca122d59b81e7635be5e5aeda8abeb
> Signed-off-by: Rex Zhu 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c |  6 +--
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 51 
> +--
>  drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h |  2 +
>  3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> index fa6baf3..e2f0507 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> @@ -155,9 +155,6 @@ static int amdgpu_pp_sw_init(void *handle)
> ret = adev->powerplay.ip_funcs->sw_init(
> adev->powerplay.pp_handle);
>
> -   if (adev->pp_enabled)
> -   adev->pm.dpm_enabled = true;
> -
> return ret;
>  }
>
> @@ -187,6 +184,9 @@ static int amdgpu_pp_hw_init(void *handle)
> ret = adev->powerplay.ip_funcs->hw_init(
> adev->powerplay.pp_handle);
>
> +   if (amdgpu_dpm != 0)
> +   adev->pm.dpm_enabled = true;
> +
> return ret;
>  }
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index 1f49764..4a4f97b 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -41,7 +41,7 @@
>  #define PP_CHECK_HW(hwmgr) \
> do {\
> if ((hwmgr) == NULL || (hwmgr)->hwmgr_func == NULL) \
> -   return -EINVAL; \
> +   return 0;   \

Is that really the right thing to do? With it functions like
pp_dpm_get_fan_speed_percent() pp_dpm_read_sensor() will succeed but
not set the values and callers will use uninitialized data (leak
kernel stack contents, so it can be considered a security issue even).

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amd/powerplay: export a function to read fan rpm

[Grazvydas Ignotas]

Hi,

On Sun, Oct 30, 2016 at 7:10 AM, Edward O'Callaghan
<funfunc...@folklore1984.net> wrote:
> Howdy,
>
> On 10/30/2016 07:28 AM, Grazvydas Ignotas wrote:
>> Powerplay hwmgr already has an implementation, all we need to do is to call 
>> it.
>>
>> Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 18 ++
>>  drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h |  1 +
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
>> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> index 0b1f220..1f49764 100644
>> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> @@ -582,6 +582,23 @@ static int pp_dpm_get_fan_speed_percent(void *handle, 
>> uint32_t *speed)
>>   return hwmgr->hwmgr_func->get_fan_speed_percent(hwmgr, speed);
>>  }
>>
>> +static int pp_dpm_get_fan_speed_rpm(void *handle, uint32_t *rpm)
>
> why not type the handle rather than have 'void *' and a coercion (I
> didn't check the call site yet..)

Well the rest of this file is written in such a style, as well as all
the other function pointers having void * handles in the powerplay
table, so it would be an odd one standing out from the others. Same
thing for the NULL checks.

>
>> +{
>> + struct pp_hwmgr *hwmgr;
>> +
>> + if (handle == NULL)
>
> if (!handle)
>
>> + return -EINVAL;
>> +
>> + hwmgr = ((struct pp_instance *)handle)->hwmgr;
>> +
>> + PP_CHECK_HW(hwmgr);
>> +
>> + if (hwmgr->hwmgr_func->get_fan_speed_rpm == NULL)
>
> if (!hwmgr->hwmgr_func->get_fan_speed_rpm)
>
>> + return -EINVAL;
>> +
>> + return hwmgr->hwmgr_func->get_fan_speed_rpm(hwmgr, rpm);
>> +}
>> +
>>  static int pp_dpm_get_temperature(void *handle)
>>  {
>>   struct pp_hwmgr  *hwmgr;
>> @@ -852,6 +869,7 @@ const struct amd_powerplay_funcs pp_dpm_funcs = {
>>   .get_fan_control_mode = pp_dpm_get_fan_control_mode,
>>   .set_fan_speed_percent = pp_dpm_set_fan_speed_percent,
>>   .get_fan_speed_percent = pp_dpm_get_fan_speed_percent,
>> + .get_fan_speed_rpm = pp_dpm_get_fan_speed_rpm,
>>   .get_pp_num_states = pp_dpm_get_pp_num_states,
>>   .get_pp_table = pp_dpm_get_pp_table,
>>   .set_pp_table = pp_dpm_set_pp_table,
>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h 
>> b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
>> index eb3e83d..2892b4e 100644
>> --- a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
>> @@ -349,6 +349,7 @@ struct amd_powerplay_funcs {
>>   int (*get_fan_control_mode)(void *handle);
>>   int (*set_fan_speed_percent)(void *handle, uint32_t percent);
>>   int (*get_fan_speed_percent)(void *handle, uint32_t *speed);
>> + int (*get_fan_speed_rpm)(void *handle, uint32_t *rpm);
>>   int (*get_pp_num_states)(void *handle, struct pp_states_info *data);
>>   int (*get_pp_table)(void *handle, char **table);
>>   int (*set_pp_table)(void *handle, const char *buf, size_t size);
>>
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay: don't succeed in getters if fan is missing

[Grazvydas Ignotas]

Otherwise callers end up using uninitialized data.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c
index fb6c6f6..29d0319 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c
@@ -30,7 +30,7 @@ int smu7_fan_ctrl_get_fan_speed_info(struct pp_hwmgr *hwmgr,
struct phm_fan_speed_info *fan_speed_info)
 {
if (hwmgr->thermal_controller.fanInfo.bNoFan)
-   return 0;
+   return -ENODEV;
 
fan_speed_info->supports_percent_read = true;
fan_speed_info->supports_percent_write = true;
@@ -60,7 +60,7 @@ int smu7_fan_ctrl_get_fan_speed_percent(struct pp_hwmgr 
*hwmgr,
uint64_t tmp64;
 
if (hwmgr->thermal_controller.fanInfo.bNoFan)
-   return 0;
+   return -ENODEV;
 
duty100 = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
CG_FDO_CTRL1, FMAX_DUTY100);
@@ -89,7 +89,7 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, 
uint32_t *speed)
if (hwmgr->thermal_controller.fanInfo.bNoFan ||
(hwmgr->thermal_controller.fanInfo.
ucTachometerPulsesPerRevolution == 0))
-   return 0;
+   return -ENODEV;
 
tach_period = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, 
CGS_IND_REG__SMC,
CG_TACH_STATUS, TACH_PERIOD);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/amdgpu: expose fan rpm though hwmon

[Grazvydas Ignotas]

Only for cards that are supported by powerplay.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h |  5 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c  | 21 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
index bd85e35..e45bd05 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
@@ -317,6 +317,11 @@ struct amdgpu_dpm_funcs {
  
(adev)->powerplay.pp_funcs->get_fan_speed_percent((adev)->powerplay.pp_handle, 
(s)) : \
  (adev)->pm.funcs->get_fan_speed_percent((adev), (s)))
 
+#define amdgpu_dpm_get_fan_speed_rpm(adev, s) \
+   ((adev)->pp_enabled ?   \
+ 
(adev)->powerplay.pp_funcs->get_fan_speed_rpm((adev)->powerplay.pp_handle, (s)) 
: \
+ -EINVAL)
+
 #define amdgpu_dpm_get_sclk(adev, l) \
((adev)->pp_enabled ?   \
  (adev)->powerplay.pp_funcs->get_sclk((adev)->powerplay.pp_handle, 
(l)) : \
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 274f330..723ae68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -737,6 +737,21 @@ static ssize_t amdgpu_hwmon_get_pwm1(struct device *dev,
return sprintf(buf, "%i\n", speed);
 }
 
+static ssize_t amdgpu_hwmon_get_fan1_input(struct device *dev,
+  struct device_attribute *attr,
+  char *buf)
+{
+   struct amdgpu_device *adev = dev_get_drvdata(dev);
+   int err;
+   u32 speed;
+
+   err = amdgpu_dpm_get_fan_speed_rpm(adev, );
+   if (err)
+   return err;
+
+   return sprintf(buf, "%i\n", speed);
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, amdgpu_hwmon_show_temp, NULL, 
0);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, amdgpu_hwmon_show_temp_thresh, 
NULL, 0);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO, 
amdgpu_hwmon_show_temp_thresh, NULL, 1);
@@ -744,6 +759,7 @@ static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, 
amdgpu_hwmon_get_pwm1, amdgpu
 static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR, 
amdgpu_hwmon_get_pwm1_enable, amdgpu_hwmon_set_pwm1_enable, 0);
 static SENSOR_DEVICE_ATTR(pwm1_min, S_IRUGO, amdgpu_hwmon_get_pwm1_min, NULL, 
0);
 static SENSOR_DEVICE_ATTR(pwm1_max, S_IRUGO, amdgpu_hwmon_get_pwm1_max, NULL, 
0);
+static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, amdgpu_hwmon_get_fan1_input, 
NULL, 0);
 
 static struct attribute *hwmon_attributes[] = {
_dev_attr_temp1_input.dev_attr.attr,
@@ -753,6 +769,7 @@ static struct attribute *hwmon_attributes[] = {
_dev_attr_pwm1_enable.dev_attr.attr,
_dev_attr_pwm1_min.dev_attr.attr,
_dev_attr_pwm1_max.dev_attr.attr,
+   _dev_attr_fan1_input.dev_attr.attr,
NULL
 };
 
@@ -804,6 +821,10 @@ static umode_t hwmon_attributes_visible(struct kobject 
*kobj,
 attr == _dev_attr_pwm1_min.dev_attr.attr))
return 0;
 
+   /* requires powerplay */
+   if (attr == _dev_attr_fan1_input.dev_attr.attr)
+   return 0;
+
return effective_mode;
 }
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/2] drm/amd/powerplay: export a function to read fan rpm

[Grazvydas Ignotas]

Powerplay hwmgr already has an implementation, all we need to do is to call it.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 18 ++
 drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index 0b1f220..1f49764 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -582,6 +582,23 @@ static int pp_dpm_get_fan_speed_percent(void *handle, 
uint32_t *speed)
return hwmgr->hwmgr_func->get_fan_speed_percent(hwmgr, speed);
 }
 
+static int pp_dpm_get_fan_speed_rpm(void *handle, uint32_t *rpm)
+{
+   struct pp_hwmgr *hwmgr;
+
+   if (handle == NULL)
+   return -EINVAL;
+
+   hwmgr = ((struct pp_instance *)handle)->hwmgr;
+
+   PP_CHECK_HW(hwmgr);
+
+   if (hwmgr->hwmgr_func->get_fan_speed_rpm == NULL)
+   return -EINVAL;
+
+   return hwmgr->hwmgr_func->get_fan_speed_rpm(hwmgr, rpm);
+}
+
 static int pp_dpm_get_temperature(void *handle)
 {
struct pp_hwmgr  *hwmgr;
@@ -852,6 +869,7 @@ const struct amd_powerplay_funcs pp_dpm_funcs = {
.get_fan_control_mode = pp_dpm_get_fan_control_mode,
.set_fan_speed_percent = pp_dpm_set_fan_speed_percent,
.get_fan_speed_percent = pp_dpm_get_fan_speed_percent,
+   .get_fan_speed_rpm = pp_dpm_get_fan_speed_rpm,
.get_pp_num_states = pp_dpm_get_pp_num_states,
.get_pp_table = pp_dpm_get_pp_table,
.set_pp_table = pp_dpm_set_pp_table,
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h 
b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
index eb3e83d..2892b4e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
@@ -349,6 +349,7 @@ struct amd_powerplay_funcs {
int (*get_fan_control_mode)(void *handle);
int (*set_fan_speed_percent)(void *handle, uint32_t percent);
int (*get_fan_speed_percent)(void *handle, uint32_t *speed);
+   int (*get_fan_speed_rpm)(void *handle, uint32_t *rpm);
int (*get_pp_num_states)(void *handle, struct pp_states_info *data);
int (*get_pp_table)(void *handle, char **table);
int (*set_pp_table)(void *handle, const char *buf, size_t size);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd: fix scheduler fence teardown order

[Grazvydas Ignotas]

On Fri, Oct 28, 2016 at 6:08 PM, Christian König
<deathsim...@vodafone.de> wrote:
> From: Christian König <christian.koe...@amd.com>
>
> Some fences might be alive even after we have stopped the scheduler leading
> to warnings about leaked objects from the SLUB allocator.
>
> Fix this by allocating/freeing the SLUB allocator from the module
> init/finfi functions just like we do it for hw fences.
>
> Reported-by: Grazvydas Ignotas <nota...@gmail.com>
> Signed-off-by: Christian König <christian.koe...@amd.com>
> ---

...

> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h 
> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
> index 7cbbbfb..28a0d8f 100644
> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
> @@ -145,6 +145,9 @@ void amd_sched_entity_fini(struct amd_gpu_scheduler 
> *sched,
>struct amd_sched_entity *entity);
>  void amd_sched_entity_push_job(struct amd_sched_job *sched_job);
>
> +int amd_sched_fence_slab_init(void);
> +void amd_sched_fence_slab_fini(void);
> +
>  struct amd_sched_fence *amd_sched_fence_create(
> struct amd_sched_entity *s_entity, void *owner);
>  void amd_sched_fence_scheduled(struct amd_sched_fence *fence);
> diff --git a/drivers/gpu/drm/amd/scheduler/sched_fence.c 
> b/drivers/gpu/drm/amd/scheduler/sched_fence.c
> index 3653b5a..8c7b5ee 100644
> --- a/drivers/gpu/drm/amd/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/amd/scheduler/sched_fence.c
> @@ -27,6 +27,25 @@
>  #include 
>  #include "gpu_scheduler.h"
>
> +struct kmem_cache *sched_fence_slab;

Can become static now I guess?


Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 答复: [PATCH] drm/amdgpu: fix fence slab teardown

[Grazvydas Ignotas]

On Mon, Oct 24, 2016 at 6:35 AM, Qu, Jim  wrote:
> I did observed the issue when replace kernel module use DKMS, and it maybe 
> get error at reboot, got calltrace:
>
> [ 3529.525360] 
> =
> [ 3529.525361] BUG amd_sched_fence (Tainted: GB  OE    ): 
> Objects remaining in amd_sched_fence on kmem_cache_close()
> [ 3529.525361] 
> -
> [ 3529.525361]
> [ 3529.525361] INFO: Slab 0xea94b200 objects=25 used=2 
> fp=0x8800252c9180 flags=0x1f4080
> [ 3529.525362] CPU: 0 PID: 18523 Comm: reboot Tainted: GB  OE  
>    3.10.0-512.el7.x86_64 #1
> [ 3529.525362] Hardware name: ASUS All Series/Z87-PLUS, BIOS 1802 01/28/2014
> [ 3529.525363]  ea94b200 b3b19dcf 880160827b50 
> 81685e8c
> [ 3529.525363]  880160827c28 811d9e34 8820 
> 880160827c38
> [ 3529.525364]  880160827be8 656a624f818de5f0 616d657220737463 
> 6e6920676e696e69
> [ 3529.525364] Call Trace:
> [ 3529.525365]  [] dump_stack+0x19/0x1b
> [ 3529.525366]  [] slab_err+0xb4/0xe0
> [ 3529.525367]  [] ? vprintk_default+0x29/0x40
> [ 3529.525368]  [] ? printk+0x5e/0x75
> [ 3529.525369]  [] ? __kmalloc+0x1f3/0x240
> [ 3529.525370]  [] ? kmem_cache_close+0x12b/0x2f0
> [ 3529.525370]  [] kmem_cache_close+0x14c/0x2f0
> [ 3529.525371]  [] __kmem_cache_shutdown+0x14/0x80
> [ 3529.525372]  [] kmem_cache_destroy+0x44/0xf0
> [ 3529.525387]  [] amd_sched_fini+0x3c/0x40 [amdgpu]
> [ 3529.525395]  [] amdgpu_fence_driver_fini+0x7a/0x110 
> [amdgpu]
> [ 3529.525403]  [] amdgpu_device_fini+0x3d/0x1f0 [amdgpu]
> [ 3529.525411]  [] amdgpu_driver_unload_kms+0x43/0x80 
> [amdgpu]
> [ 3529.525416]  [] drm_dev_unregister+0x29/0xb0 [drm]
> [ 3529.525422]  [] drm_put_dev+0x23/0x70 [drm]
> [ 3529.525429]  [] amdgpu_pci_shutdown+0x1d/0x20 [amdgpu]
> [ 3529.525430]  [] pci_device_shutdown+0x36/0x70
> [ 3529.525431]  [] device_shutdown+0xc8/0x180
> [ 3529.525432]  [] kernel_restart_prepare+0x36/0x40
> [ 3529.525433]  [] kernel_restart+0x12/0x60
> [ 3529.525433]  [] SYSC_reboot+0x229/0x260
> [ 3529.525435]  [] ? __do_page_fault+0x171/0x450
> [ 3529.525436]  [] SyS_reboot+0xe/0x10
> [ 3529.525437]  [] system_call_fastpath+0x16/0x1b
> [ 3529.525438] INFO: Object 0x8800252c8a00 @offset=2560
> [ 3529.525438] INFO: Object 0x8800252c9540 @offset=5440
>
>
> Do these series patches fix this issue?

Yes, but only partially - there are still some leaked objects left.
When SLUB_DEBUG is set, you can also set CONFIG_SLUB_DEBUG_ON or add
"slub_debug" to kernel command line to see the leak backtraces.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: update kernel-doc for some functions

[Grazvydas Ignotas]

The names were wrong.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/scheduler/sched_fence.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/scheduler/sched_fence.c 
b/drivers/gpu/drm/amd/scheduler/sched_fence.c
index 6b63bea..3653b5a 100644
--- a/drivers/gpu/drm/amd/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/amd/scheduler/sched_fence.c
@@ -103,7 +103,7 @@ static void amd_sched_fence_free(struct rcu_head *rcu)
 }
 
 /**
- * amd_sched_fence_release - callback that fence can be freed
+ * amd_sched_fence_release_scheduled - callback that fence can be freed
  *
  * @fence: fence
  *
@@ -118,7 +118,7 @@ static void amd_sched_fence_release_scheduled(struct fence 
*f)
 }
 
 /**
- * amd_sched_fence_release_scheduled - drop extra reference
+ * amd_sched_fence_release_finished - drop extra reference
  *
  * @f: fence
  *
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix sched fence slab teardown

[Grazvydas Ignotas]

To free fences, call_rcu() is used, which calls amd_sched_fence_free()
after a grace period. During teardown, there is no guarantee all
callbacks have finished, so sched_fence_slab may be destroyed before
all fences have been freed. If we are lucky, this results in some slab
warnings, if not, we get a crash in one of rcu threads because callback
is called after amdgpu has already been unloaded.

Fix it with a rcu_barrier().

Fixes: 189e0fb76304 ("drm/amdgpu: RCU protected amd_sched_fence_release")
Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
index 963a24d..910b8d5 100644
--- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
@@ -645,6 +645,7 @@ void amd_sched_fini(struct amd_gpu_scheduler *sched)
 {
if (sched->thread)
kthread_stop(sched->thread);
+   rcu_barrier();
if (atomic_dec_and_test(_fence_slab_ref))
kmem_cache_destroy(sched_fence_slab);
 }
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix a vm_flush fence leak

[Grazvydas Ignotas]

Looks like .last_flush reference is left at teardown.
Leak reported by CONFIG_SLUB_DEBUG.

Fixes: 41d9eb2c5a2a ("drm/amdgpu: add a fence after the VM flush")
Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ded57dd..d6c2839 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1774,5 +1774,6 @@ void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
fence_put(adev->vm_manager.ids[i].first);
amdgpu_sync_free(>vm_manager.ids[i].active);
fence_put(id->flushed_updates);
+   fence_put(id->last_flush);
}
 }
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix fence slab teardown

[Grazvydas Ignotas]

To free fences, call_rcu() is used, which calls amdgpu_fence_free()
after a grace period. During teardown, there is no guarantee all
callbacks have finished, so amdgpu_fence_slab may be destroyed before
all fences have been freed. If we are lucky, this results in some slab
warnings, if not, we get a crash in one of rcu threads because callback
is called after amdgpu has already been unloaded.

Fix it with a rcu_barrier().

Fixes: b44135351a3a ("drm/amdgpu: RCU protected amdgpu_fence_release")
Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 3a2e42f..77b34ec 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -68,6 +68,7 @@ int amdgpu_fence_slab_init(void)
 
 void amdgpu_fence_slab_fini(void)
 {
+   rcu_barrier();
kmem_cache_destroy(amdgpu_fence_slab);
 }
 /*
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: release parent fence reference

[Grazvydas Ignotas]

It's done in amd_sched_hw_job_reset(), but not in normal job processing.
Leak reported by CONFIG_SLUB_DEBUG.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 CONFIG_SLUB_DEBUG reports more leaks related to ioctls,
 but I was unable to track them down...

 drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
index 910b8d5..cfb686e 100644
--- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
@@ -522,6 +522,8 @@ static void amd_sched_process_job(struct fence *f, struct 
fence_cb *cb)
 
trace_amd_sched_process_job(s_fence);
fence_put(_fence->finished);
+   fence_put(s_fence->parent);
+   s_fence->parent = NULL;
wake_up_interruptible(>wake_up_worker);
 }
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running

[Grazvydas Ignotas]

Hi,

On Thu, Oct 13, 2016 at 10:45 AM, Zhu, Rex  wrote:
>
> The attached patches were also for this issue.
> Disable dpm when rmmod amdgpu.

It works for modprobe-rmmod-modprobe test, thanks.

However with GPU passthrough (giving control of the GPU to a Windows
virtual machine using iommu, then shutting down the VM and loading
amdgpu) the problem is still there, same backtrace as in my commit
message. It seems the Windows driver leaves DPM enabled on shutdown.
With my patch the crash goes away.

It would be nice to have GPU passthrough working, this is the only
issue that breaks it. Windows can drive the GPU after amdgpu fine
already, only amdgpu has problems to run after Windows.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdgpu: used cached gca values for vi_read_register

[Grazvydas Ignotas]

On Wed, Oct 12, 2016 at 2:48 AM, Andy Furniss  wrote:
>
> I still can't shutdown/reboot
> as in https://bugs.freedesktop.org/show_bug.cgi?id=98200
> which is fixed for radeon, but apparently not (for me at least) with amdgpu.

You probably need a951ed85abd46 that went to 4.8-fixes and is not part
of 4.9-wip.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/7] drm/amdgpu: remove adev pointer from struct amdgpu_bo

[Grazvydas Ignotas]

On Thu, Sep 29, 2016 at 10:52 AM, Christian König
 wrote:
> From: Christian König 
>
> It's completely pointsless to have two pointers to the
> device in the same structur.

Several typos here...

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/9] drm/amdgpu:keep bo pinned in prefered domain

[Grazvydas Ignotas]

Hi,

this patch causes failure on my polaris10 card:
[drm:gfx_v8_0_ring_test_ring] *ERROR* amdgpu: ring 0 test failed
(scratch(0xC040)=0xCAFEDEAD)
[drm:amdgpu_init] *ERROR* hw_init of IP block  failed -22
amdgpu :01:00.0: amdgpu_init failed

Gražvydas

On Wed, Sep 28, 2016 at 11:36 AM, Monk Liu  wrote:
> From: Frank Min 
>
> Change-Id: I87fae602a221902f58f61069c18930627012
> Signed-off-by: Frank Min 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  mode change 100644 => 100755 drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> old mode 100644
> new mode 100755
> index ab2d7fb..7f79323
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -240,7 +240,7 @@ static int amdgpu_cgs_gmap_gpu_mem(struct cgs_device 
> *cgs_device, cgs_handle_t h
> r = amdgpu_bo_reserve(obj, false);
> if (unlikely(r != 0))
> return r;
> -   r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT,
> +   r = amdgpu_bo_pin_restricted(obj, obj->prefered_domains,
>  min_offset, max_offset, mcaddr);
> amdgpu_bo_unreserve(obj);
> return r;
> --
> 1.9.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/dce11: add missing drm_mode_config_cleanup call

[Grazvydas Ignotas]

All other amdgpu/dce_v* files have this call, it's only mysteriously
missing from dce_v11_0.c since the file was added and causes leaks.

Fixes: aaa36a976bbb ("drm/amdgpu: Add initial VI support")
Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c 
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 0bed302..2696428 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -3075,6 +3075,7 @@ static int dce_v11_0_sw_fini(void *handle)
 
dce_v11_0_afmt_fini(adev);
 
+   drm_mode_config_cleanup(adev->ddev);
adev->mode_info.mode_config_initialized = false;
 
return 0;
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: warn if dp aux is still attached on free

[Grazvydas Ignotas]

If this happens (and it recently did), we free a structure while part of
it is still in use, which results in non-obvious crashes. The way it's
detached is not trivial (DRM core has to call the connector .destroy
callback and things must be torn down in the right order), so better
detect it and warn early.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c| 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index ff0b55a..76a7830 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -769,8 +769,10 @@ static void amdgpu_connector_destroy(struct drm_connector 
*connector)
 {
struct amdgpu_connector *amdgpu_connector = 
to_amdgpu_connector(connector);
 
-   if (amdgpu_connector->ddc_bus->has_aux)
+   if (amdgpu_connector->ddc_bus->has_aux) {
drm_dp_aux_unregister(_connector->ddc_bus->aux);
+   amdgpu_connector->ddc_bus->has_aux = false;
+   }
amdgpu_connector_free_edid(connector);
kfree(amdgpu_connector->con_priv);
drm_connector_unregister(connector);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
index 34bab61..91d3673 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
@@ -220,6 +220,7 @@ void amdgpu_i2c_destroy(struct amdgpu_i2c_chan *i2c)
 {
if (!i2c)
return;
+   WARN_ON(i2c->has_aux);
i2c_del_adapter(>adapter);
kfree(i2c);
 }
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: also track late init state

[Grazvydas Ignotas]

Successful sw_init() and hw_init() states are tracked, but not
late_init(). Various error paths may result in amdgpu_fini() being
called before .late init is done, so late_init needs to be tracked
to avoid unexpected or multiple .late_fini() calls.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index b9cc003..4e510092 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1943,6 +1943,7 @@ struct amdgpu_ip_block_status {
bool valid;
bool sw;
bool hw;
+   bool late_initialized;
bool hang;
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 9933ab7..0c57898 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1450,6 +1450,7 @@ static int amdgpu_late_init(struct amdgpu_device *adev)
DRM_ERROR("late_init of IP block <%s> failed 
%d\n", adev->ip_blocks[i].funcs->name, r);
return r;
}
+   adev->ip_block_status[i].late_initialized = true;
}
}
 
@@ -1495,8 +1496,11 @@ static int amdgpu_fini(struct amdgpu_device *adev)
}
 
for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
+   if (!adev->ip_block_status[i].late_initialized)
+   continue;
if (adev->ip_blocks[i].funcs->late_fini)
adev->ip_blocks[i].funcs->late_fini((void *)adev);
+   adev->ip_block_status[i].late_initialized = false;
}
 
return 0;
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: disable CRTCs before teardown

[Grazvydas Ignotas]

On Mon, Sep 26, 2016 at 8:29 PM, Lukas Wunner <lu...@wunner.de> wrote:
> On Sun, Sep 25, 2016 at 11:34:48PM +0300, Grazvydas Ignotas wrote:
>> Some code called by drm_crtc_force_disable_all() wants to wait for all
>> fences, so only do fence teardown after CRTCs are disabled.
>
> Ugh, how embarrassing, that was added by me.
>
> Do you have a BUG splat (e.g. soft lockup) for this?  I'd be curious to
> see exactly where things explode, would also be good to have that in
> the commit message.

It can be seen here, the first one:
https://bugs.freedesktop.org/attachment.cgi?id=126769
I'm not sure it's that useful to have the full BUG dump with all the
registers, pointers and stuff in the commit message, like some people
like to have, most of that is only relevant for my specific build.

Gražvydas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/i2c: add const where appropriate

[Grazvydas Ignotas]

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c | 14 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.h | 14 +++---
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
index c93a92a..34bab61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
@@ -158,8 +158,8 @@ static const struct i2c_algorithm amdgpu_atombios_i2c_algo 
= {
 };
 
 struct amdgpu_i2c_chan *amdgpu_i2c_create(struct drm_device *dev,
-   struct amdgpu_i2c_bus_rec *rec,
-   const char *name)
+ const struct amdgpu_i2c_bus_rec *rec,
+ const char *name)
 {
struct amdgpu_i2c_chan *i2c;
int ret;
@@ -249,8 +249,8 @@ void amdgpu_i2c_fini(struct amdgpu_device *adev)
 
 /* Add additional buses */
 void amdgpu_i2c_add(struct amdgpu_device *adev,
-struct amdgpu_i2c_bus_rec *rec,
-const char *name)
+   const struct amdgpu_i2c_bus_rec *rec,
+   const char *name)
 {
struct drm_device *dev = adev->ddev;
int i;
@@ -266,7 +266,7 @@ void amdgpu_i2c_add(struct amdgpu_device *adev,
 /* looks up bus based on id */
 struct amdgpu_i2c_chan *
 amdgpu_i2c_lookup(struct amdgpu_device *adev,
-  struct amdgpu_i2c_bus_rec *i2c_bus)
+ const struct amdgpu_i2c_bus_rec *i2c_bus)
 {
int i;
 
@@ -336,7 +336,7 @@ static void amdgpu_i2c_put_byte(struct amdgpu_i2c_chan 
*i2c_bus,
 
 /* ddc router switching */
 void
-amdgpu_i2c_router_select_ddc_port(struct amdgpu_connector *amdgpu_connector)
+amdgpu_i2c_router_select_ddc_port(const struct amdgpu_connector 
*amdgpu_connector)
 {
u8 val;
 
@@ -365,7 +365,7 @@ amdgpu_i2c_router_select_ddc_port(struct amdgpu_connector 
*amdgpu_connector)
 
 /* clock/data router switching */
 void
-amdgpu_i2c_router_select_cd_port(struct amdgpu_connector *amdgpu_connector)
+amdgpu_i2c_router_select_cd_port(const struct amdgpu_connector 
*amdgpu_connector)
 {
u8 val;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.h
index d81e19b..63c2ff7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.h
@@ -25,20 +25,20 @@
 #define __AMDGPU_I2C_H__
 
 struct amdgpu_i2c_chan *amdgpu_i2c_create(struct drm_device *dev,
-   struct amdgpu_i2c_bus_rec *rec,
-   const char *name);
+ const struct amdgpu_i2c_bus_rec *rec,
+ const char *name);
 void amdgpu_i2c_destroy(struct amdgpu_i2c_chan *i2c);
 void amdgpu_i2c_init(struct amdgpu_device *adev);
 void amdgpu_i2c_fini(struct amdgpu_device *adev);
 void amdgpu_i2c_add(struct amdgpu_device *adev,
-struct amdgpu_i2c_bus_rec *rec,
-const char *name);
+   const struct amdgpu_i2c_bus_rec *rec,
+   const char *name);
 struct amdgpu_i2c_chan *
 amdgpu_i2c_lookup(struct amdgpu_device *adev,
-  struct amdgpu_i2c_bus_rec *i2c_bus);
+ const struct amdgpu_i2c_bus_rec *i2c_bus);
 void
-amdgpu_i2c_router_select_ddc_port(struct amdgpu_connector *amdgpu_connector);
+amdgpu_i2c_router_select_ddc_port(const struct amdgpu_connector *connector);
 void
-amdgpu_i2c_router_select_cd_port(struct amdgpu_connector *amdgpu_connector);
+amdgpu_i2c_router_select_cd_port(const struct amdgpu_connector *connector);
 
 #endif
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/vce3: don't forget to tear down some rings

[Grazvydas Ignotas]

We can use .num_rings for that.

Fixes: 6f0359ff7307 ("vce3: add support for third vce ring")
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
index 06b94c1..121bd6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -210,6 +210,8 @@ int amdgpu_vce_sw_init(struct amdgpu_device *adev, unsigned 
long size)
  */
 int amdgpu_vce_sw_fini(struct amdgpu_device *adev)
 {
+   unsigned i;
+
if (adev->vce.vcpu_bo == NULL)
return 0;
 
@@ -217,8 +219,8 @@ int amdgpu_vce_sw_fini(struct amdgpu_device *adev)
 
amdgpu_bo_unref(>vce.vcpu_bo);
 
-   amdgpu_ring_fini(>vce.ring[0]);
-   amdgpu_ring_fini(>vce.ring[1]);
+   for (i = 0; i < adev->vce.num_rings; i++)
+   amdgpu_ring_fini(>vce.ring[i]);
 
release_firmware(adev->vce.fw);
mutex_destroy(>vce.idle_mutex);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: don't leave dangling pointers around

[Grazvydas Ignotas]

Right now it's possible to trigger fence_drv.fences[] dereference after
the array has been freed. While the real problem is elsewhere, this still
results in confusing errors that depend on how the freed memory was
reused (I've seen "kernel tried to execute NX-protected page"), it's
better to clear them and get NULL dereference so that it's obvious what's
going wrong.

Signed-off-by: Grazvydas Ignotas <nota...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 17e1362..e203e55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -60,6 +60,7 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev, struct 
amdgpu_ctx *ctx)
amd_sched_entity_fini(>rings[j]->sched,
  >rings[j].entity);
kfree(ctx->fences);
+   ctx->fences = NULL;
return r;
}
return 0;
@@ -77,6 +78,7 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx)
for (j = 0; j < amdgpu_sched_jobs; ++j)
fence_put(ctx->rings[i].fences[j]);
kfree(ctx->fences);
+   ctx->fences = NULL;
 
for (i = 0; i < adev->num_rings; i++)
amd_sched_entity_fini(>rings[i]->sched,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 0b109ae..3a2e42f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -454,6 +454,7 @@ void amdgpu_fence_driver_fini(struct amdgpu_device *adev)
for (j = 0; j <= ring->fence_drv.num_fences_mask; ++j)
fence_put(ring->fence_drv.fences[j]);
kfree(ring->fence_drv.fences);
+   ring->fence_drv.fences = NULL;
ring->fence_drv.initialized = false;
}
 }
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx