Re: [PATCH] drm/amdgpu: Verify root PD is mapped into kernel address space.

[zhoucm1]

On 2018年07月05日 03:49, Andrey Grodzovsky wrote:

Problem: When PD/PT update made by CPU root PD was not yet mapped causing
page fault.

Fix: Move amdgpu_bo_kmap into amdgpu_vm_bo_base_init to cover
all cases and avoid code duplication with amdgpu_vm_alloc_levels.

Link: https://bugs.freedesktop.org/show_bug.cgi?id=107065
Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 47 ++
  1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 845f73a..f546afa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -143,25 +143,36 @@ struct amdgpu_prt_cb {
   * Initialize a bo_va_base structure and add it to the appropriate lists
   *
   */
-static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+static int amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
   struct amdgpu_vm *vm,
   struct amdgpu_bo *bo)
  {
+   int r = 0;
base->vm = vm;
base->bo = bo;
INIT_LIST_HEAD(>bo_list);
INIT_LIST_HEAD(>vm_status);
  
  	if (!bo)

-   return;
+   return r;
+
list_add_tail(>bo_list, >va);
  
+	if (vm->use_cpu_for_update && bo->tbo.type == ttm_bo_type_kernel) {

+   r = amdgpu_bo_kmap(bo, NULL);
+   if (r) {
+   amdgpu_bo_unref(>shadow);
+   amdgpu_bo_unref();
I feel these two lines should move out of helper function, ref/unref 
should appear in where used with pair.


Regards,
David Zhou

+   return r;
+   }
+   }
+
if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
-   return;
+   return r;
  
  	if (bo->preferred_domains &

amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
-   return;
+   return r;
  
  	/*

 * we checked all the prerequisites, but it looks like this per vm bo
@@ -169,6 +180,8 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base 
*base,
 * is validated on next vm use to avoid fault.
 * */
list_move_tail(>vm_status, >evicted);
+
+   return r;
  }
  
  /**

@@ -525,21 +538,15 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device 
*adev,
return r;
}
  
-			if (vm->use_cpu_for_update) {

-   r = amdgpu_bo_kmap(pt, NULL);
-   if (r) {
-   amdgpu_bo_unref(>shadow);
-   amdgpu_bo_unref();
-   return r;
-   }
-   }
-
/* Keep a reference to the root directory to avoid
* freeing them up in the wrong order.
*/
pt->parent = amdgpu_bo_ref(parent->base.bo);
  
-			amdgpu_vm_bo_base_init(>base, vm, pt);

+   r = amdgpu_vm_bo_base_init(>base, vm, pt);
+   if (r)
+   return r;
+
list_move(>base.vm_status, >relocated);
}
  
@@ -1992,12 +1999,17 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,

  struct amdgpu_bo *bo)
  {
struct amdgpu_bo_va *bo_va;
+   int r;
  
  	bo_va = kzalloc(sizeof(struct amdgpu_bo_va), GFP_KERNEL);

if (bo_va == NULL) {
return NULL;
}
-   amdgpu_vm_bo_base_init(_va->base, vm, bo);
+
+   if ((r = amdgpu_vm_bo_base_init(_va->base, vm, bo))) {
+   WARN_ONCE(1,"r = %d\n", r);
+   return NULL;
+   }
  
  	bo_va->ref_count = 1;

INIT_LIST_HEAD(_va->valids);
@@ -2613,7 +2625,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct 
amdgpu_vm *vm,
if (r)
goto error_unreserve;
  
-	amdgpu_vm_bo_base_init(>root.base, vm, root);

+   r = amdgpu_vm_bo_base_init(>root.base, vm, root);
+   if (r)
+   goto error_unreserve;
+
amdgpu_bo_unreserve(vm->root.base.bo);
  
  	if (pasid) {


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

Re: [PATCH v2 2/2] drm/admgpu: Present amdgpu_task_info in VM_FAULTS.

[zhoucm1]

On 2018年07月04日 23:04, Andrey Grodzovsky wrote:

Extract and present the reposnsible process and thread when
VM_FAULT happens.

Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  | 10 --
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  |  9 +++--
  3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 7a625f3..1c483ea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -187,6 +187,18 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser 
*p, void *data)
if (p->uf_entry.robj)
p->job->uf_addr = uf_offset;
kfree(chunk_array);
+
+   /* Use this opportunity to fill in task info for the vm */
+   if (!vm->task_info.pid) {
+   vm->task_info.pid = current->pid;
+   get_task_comm(vm->task_info.task_name, current);
+
+   if (current->group_leader->mm == current->mm) {
+   vm->task_info.tgid = current->group_leader->pid;
+   get_task_comm(vm->task_info.process_name, 
current->group_leader);
+   }
+   }
+

you can wrap this segment to  a function like amdgpu_vm_set_task_info.



return 0;
  
  free_all_kdata:

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 08753e7..7ad19f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -46,6 +46,7 @@
  
  #include "ivsrcid/ivsrcid_vislands30.h"
  
+#include "amdgpu_vm.h"
  
  static void gmc_v8_0_set_gmc_funcs(struct amdgpu_device *adev);

  static void gmc_v8_0_set_irq_funcs(struct amdgpu_device *adev);
@@ -1449,8 +1450,13 @@ static int gmc_v8_0_process_interrupt(struct 
amdgpu_device *adev,
gmc_v8_0_set_fault_enable_default(adev, false);
  
  	if (printk_ratelimit()) {

-   dev_err(adev->dev, "GPU fault detected: %d 0x%08x\n",
-   entry->src_id, entry->src_data[0]);
+   struct amdgpu_task_info task_info = { 0 };
+
+   amdgpu_vm_task_info(adev, entry->pasid, _info);

you can rename this function to amdgpu_vm_get_task_info.

general, it looks very good to me and does what I want to do before.

Thanks,
David Zhou

+
+   dev_err(adev->dev, "GPU fault detected: %d 0x%08x for process %s pid 
%d thread %s pid %d\n",
+   entry->src_id, entry->src_data[0], 
task_info.process_name,
+   task_info.tgid, task_info.task_name, task_info.pid);
dev_err(adev->dev, "  VM_CONTEXT1_PROTECTION_FAULT_ADDR   
0x%08X\n",
addr);
dev_err(adev->dev, "  VM_CONTEXT1_PROTECTION_FAULT_STATUS 
0x%08X\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 691a659..384a89c 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -259,11 +259,16 @@ static int gmc_v9_0_process_interrupt(struct 
amdgpu_device *adev,
}
  
  	if (printk_ratelimit()) {

+   struct amdgpu_task_info task_info = { 0 };
+
+   amdgpu_vm_task_info(adev, entry->pasid, _info);
+
dev_err(adev->dev,
-   "[%s] VMC page fault (src_id:%u ring:%u vmid:%u 
pasid:%u)\n",
+   "[%s] VMC page fault (src_id:%u ring:%u vmid:%u pasid:%u, 
for process %s pid %d thread %s pid %d\n)\n",
entry->vmid_src ? "mmhub" : "gfxhub",
entry->src_id, entry->ring_id, entry->vmid,
-   entry->pasid);
+   entry->pasid, task_info.process_name, task_info.tgid,
+   task_info.task_name, task_info.pid);
dev_err(adev->dev, "  at page 0x%016llx from %d\n",
addr, entry->client_id);
if (!amdgpu_sriov_vf(adev))


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

Proposal to merge KFD into amdgpu

[Felix Kuehling]

Since KFD is only supported by a single GPU driver now (amdgpu), it
makes sense to merge the two. This has been raised on the amd-gfx list
before and I've been putting it off to avoid more churn while I was
working on upstreaming KFD. Now seems a good time to pick this up again.

At this stage there are some things that I don't expect to change
significantly:

  * Directory structure
  * KFD function naming conventions
  * KFD device and sysfs interfaces

This is a rough overview of the changes I have in mind. We should be
able to implement these step-by-step and minimize disruption:

1. Change the build system to build KFD into amdgpu.ko

This should make KFD similar to DAL or powerplay. It's still a mostly
separate code base and Makefile with its own directory, but gets linked
into amdgpu.ko.

In the kernel configuration HSA_AMD would become a boolean option under
DRM_AMDGPU that controls whether KFD functionality gets built into amdgpu.

Any code inside #if defined(CONFIG_HSA_AMD_MODULE) can be removed.

2. Simplify the kfd2kgd and kgd2kfg interfaces

Function pointers in struct kgd2kfd_calls are no longer needed. These
functions can be called directly from amdgpu.

Hardware-independent function pointers in kfd2kgd_calls are no longer
needed. These function can be called directly from amdkfd. Some of the
function pointers in kfd2kgd_calls are used for hardware abstraction
with different implementations for each GFX HW generation. These will
need to remain function pointers.

At some later stage, the HW-specific functions could be moved into
gfx_v*.c and the function pointers added to struct amdgpu_gfx. But at
this stage I think I'd leave them where they are.

3. Reduce duplicate tracking of device and BO structures

Currently KFD and AMDGPU pretend to not know each other's data
structures. If both are in the same module, we could allow KFD to access
some amdgpu structures directly (e.g. amdgpu_device and amdgpu_bo). This
way some of the duplicate tracking of devices and buffer objects could
be eliminated.

This may present opportunities to simplify some functionality that's
currently split across both modules, such as suspend/resume, memory
management and evictions.

Some interfaces that just query information from amdgpu could be removed
if KFD can access that information directly (e.g. firmware versions, CU
info, ...).

Please let me know if you have any objections, suggestions, ideas ...

Regards,
  Felix

-- 
F e l i x   K u e h l i n g
PMTS Software Development Engineer | Linux Compute Kernel
1 Commerce Valley Dr. East, Markham, ON L3T 7X6 Canada
(O) +1(289)695-1597
   _ _   _   _   _
  / \   | \ / | |  _  \  \ _  |
 / A \  | \M/ | | |D) )  /|_| |
/_/ \_\ |_| |_| |_/ |__/ \|   facebook.com/AMD | amd.com

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

[PATCH v2] drm/amd/display: add a check for display depth validity

[mikita.lipski]

From: Mikita Lipski 

[why]
HDMI 2.0 fails to validate 4K@60 timing with 10 bpc
[how]
Adding a helper function that would verify if the display depth
assigned would pass a bandwidth validation.
Drop the display depth by one level till calculated pixel clk
is lower than maximum TMDS clk.

Bugzilla: https://bugs.freedesktop.org/106959

Signed-off-by: Mikita Lipski 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 +++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 9529043..a0f1b1d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2218,6 +2218,46 @@ get_output_color_space(const struct dc_crtc_timing 
*dc_crtc_timing)
return color_space;
 }
 
+static void reduce_mode_colour_depth(struct dc_crtc_timing *timing_out)
+{
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+
+   timing_out->display_color_depth--;
+}
+
+static void adjust_colour_depth_from_display_info(struct dc_crtc_timing 
*timing_out,
+   const struct drm_display_info 
*info)
+{
+   int normalized_clk;
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+   do {
+   normalized_clk = timing_out->pix_clk_khz;
+   /* YCbCr 4:2:0 requires additional adjustment of 1/2 */
+   if (timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420)
+   normalized_clk /= 2;
+   /* Adjusting pix clock following on HDMI spec based on colour 
depth */
+   switch (timing_out->display_color_depth) {
+   case COLOR_DEPTH_101010:
+   normalized_clk = (normalized_clk * 30) / 24;
+   break;
+   case COLOR_DEPTH_121212:
+   normalized_clk = (normalized_clk * 36) / 24;
+   break;
+   case COLOR_DEPTH_161616:
+   normalized_clk = (normalized_clk * 48) / 24;
+   break;
+   default:
+   return;
+   }
+   if (normalized_clk <= info->max_tmds_clock)
+   return;
+   reduce_mode_colour_depth(timing_out);
+
+   } while (timing_out->display_color_depth > COLOR_DEPTH_888);
+
+}
 /*/
 
 static void
@@ -2274,6 +2314,8 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
 
stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
+   if (stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   adjust_colour_depth_from_display_info(timing_out, info);
 }
 
 static void fill_audio_info(struct audio_info *audio_info,
-- 
2.7.4

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

[PATCH v2] drm/amd/display: adding ycbcr420 pixel encoding for hdmi

[mikita.lipski]

From: Mikita Lipski 

[why]
HDMI EDID's VSDB contains spectial timings for specifically
YCbCr 4:2:0 colour space. In those cases we need to verify
if the mode provided is one of the special ones has to use
YCbCr 4:2:0 pixel encoding for display info.
[how]
Verify if the mode is using specific ycbcr420 colour space with
the help of DRM helper function and assign the mode to use
ycbcr420 pixel encoding.

Signed-off-by: Mikita Lipski 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 01d14d8..9529043 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2226,6 +2226,7 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
 const struct drm_connector 
*connector)
 {
struct dc_crtc_timing *timing_out = >timing;
+   const struct drm_display_info *info = >display_info;
 
memset(timing_out, 0, sizeof(struct dc_crtc_timing));
 
@@ -2234,8 +2235,10 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
timing_out->v_border_top = 0;
timing_out->v_border_bottom = 0;
/* TODO: un-hardcode */
-
-   if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB444)
+   if (drm_mode_is_420_only(info, mode_in)
+   && stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+   else if ((connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCRCB444)
&& stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
else
-- 
2.7.4

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

Re: [PATCH] drm/amd/display: add a check for display depth validity

[Harry Wentland]

On 2018-07-04 04:57 PM, Mikita Lipski wrote:
> 
> 
> On 2018-07-04 04:51 PM, Harry Wentland wrote:
>> On 2018-07-04 04:40 PM, mikita.lip...@amd.com wrote:
>>> From: Mikita Lipski 
>>>
>>> [why]
>>> HDMI 2.0 fails to validate 4K@60 timing with 10 bpc
>>> [how]
>>> Adding a helper function that would verify if the display depth
>>> assigned would pass a bandwidth validation.
>>> Drop the display depth by one level till calculated pixel clk
>>> is lower than maximum TMDS clk.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/106959
>>>
>>> Signed-off-by: Mikita Lipski 
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 
>>> +++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index ed09e36..c572e86 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -2218,6 +2218,46 @@ get_output_color_space(const struct dc_crtc_timing 
>>> *dc_crtc_timing)
>>>   return color_space;
>>>   }
>>>   +static void reduce_mode_colour_depth(struct dc_crtc_timing *timing_out)
>>> +{
>>> +    if (timing_out->display_color_depth <= COLOR_DEPTH_888)
>>> +    return;
>>> +
>>> +    timing_out->display_color_depth--;
>>> +}
>>> +
>>> +static void check_if_display_depth_is_supported(struct dc_crtc_timing 
>>> *timing_out,
>>
>> Doesn't this update timing_out->display_color_depth? We should probably name 
>> this something else, maybe "pick_max_supported_color_depth" or something 
>> similar.
>>
>> Harry
>>
> The function reduce_mode_colour_depth introduced above is the one that 
> modifies timing_out->display_color_depth.
> 

But check_if_display_depth_is_supported here calls reduce_mode_color_depth, so 
does more than just "check". :)

Harry

> Nick
>>> +    const struct drm_display_info *info)
>>> +{
>>> +    int normalized_clk;
>>> +    if (timing_out->display_color_depth <= COLOR_DEPTH_888)
>>> +    return;
>>> +    do {
>>> +    normalized_clk = timing_out->pix_clk_khz;
>>> +    /* YCbCr 4:2:0 requires additional adjustment of 1/2 */
>>> +    if (timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420)
>>> +    normalized_clk /= 2;
>>> +    /* Adjusting pix clock following on HDMI spec based on colour 
>>> depth */
>>> +    switch (timing_out->display_color_depth) {
>>> +    case COLOR_DEPTH_101010:
>>> +    normalized_clk = (normalized_clk * 30) / 24;
>>> +    break;
>>> +    case COLOR_DEPTH_121212:
>>> +    normalized_clk = (normalized_clk * 36) / 24;
>>> +    break;
>>> +    case COLOR_DEPTH_161616:
>>> +    normalized_clk = (normalized_clk * 48) / 24;
>>> +    break;
>>> +    default:
>>> +    return;
>>> +    }
>>> +    if (normalized_clk <= info->max_tmds_clock)
>>> +    return;
>>> +    reduce_mode_colour_depth(timing_out);
>>> +
>>> +    } while (timing_out->display_color_depth > COLOR_DEPTH_888);
>>> +
>>> +}
>>>   
>>> /*/
>>>     static void
>>> @@ -2273,6 +2313,8 @@ fill_stream_properties_from_drm_display_mode(struct 
>>> dc_stream_state *stream,
>>>     stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
>>>   stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
>>> +    if (stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
>>> +    check_if_display_depth_is_supported(timing_out, info);
>>>   }
>>>     static void fill_audio_info(struct audio_info *audio_info,
>>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add a check for display depth validity

[Mikita Lipski]

On 2018-07-04 04:51 PM, Harry Wentland wrote:

On 2018-07-04 04:40 PM, mikita.lip...@amd.com wrote:

From: Mikita Lipski 

[why]
HDMI 2.0 fails to validate 4K@60 timing with 10 bpc
[how]
Adding a helper function that would verify if the display depth
assigned would pass a bandwidth validation.
Drop the display depth by one level till calculated pixel clk
is lower than maximum TMDS clk.

Bugzilla: https://bugs.freedesktop.org/106959

Signed-off-by: Mikita Lipski 
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 +++
  1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ed09e36..c572e86 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2218,6 +2218,46 @@ get_output_color_space(const struct dc_crtc_timing 
*dc_crtc_timing)
return color_space;
  }
  
+static void reduce_mode_colour_depth(struct dc_crtc_timing *timing_out)

+{
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+
+   timing_out->display_color_depth--;
+}
+
+static void check_if_display_depth_is_supported(struct dc_crtc_timing 
*timing_out,


Doesn't this update timing_out->display_color_depth? We should probably name this 
something else, maybe "pick_max_supported_color_depth" or something similar.

Harry

The function reduce_mode_colour_depth introduced above is the one that 
modifies timing_out->display_color_depth.


Nick

+   const struct drm_display_info 
*info)
+{
+   int normalized_clk;
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+   do {
+   normalized_clk = timing_out->pix_clk_khz;
+   /* YCbCr 4:2:0 requires additional adjustment of 1/2 */
+   if (timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420)
+   normalized_clk /= 2;
+   /* Adjusting pix clock following on HDMI spec based on colour 
depth */
+   switch (timing_out->display_color_depth) {
+   case COLOR_DEPTH_101010:
+   normalized_clk = (normalized_clk * 30) / 24;
+   break;
+   case COLOR_DEPTH_121212:
+   normalized_clk = (normalized_clk * 36) / 24;
+   break;
+   case COLOR_DEPTH_161616:
+   normalized_clk = (normalized_clk * 48) / 24;
+   break;
+   default:
+   return;
+   }
+   if (normalized_clk <= info->max_tmds_clock)
+   return;
+   reduce_mode_colour_depth(timing_out);
+
+   } while (timing_out->display_color_depth > COLOR_DEPTH_888);
+
+}
  
/*/
  
  static void

@@ -2273,6 +2313,8 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
  
  	stream->out_transfer_func->type = TF_TYPE_PREDEFINED;

stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
+   if (stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   check_if_display_depth_is_supported(timing_out, info);
  }
  
  static void fill_audio_info(struct audio_info *audio_info,



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

Re: [PATCH] drm/amd/display: add a check for display depth validity

[Harry Wentland]

On 2018-07-04 04:40 PM, mikita.lip...@amd.com wrote:
> From: Mikita Lipski 
> 
> [why]
> HDMI 2.0 fails to validate 4K@60 timing with 10 bpc
> [how]
> Adding a helper function that would verify if the display depth
> assigned would pass a bandwidth validation.
> Drop the display depth by one level till calculated pixel clk
> is lower than maximum TMDS clk.
> 
> Bugzilla: https://bugs.freedesktop.org/106959
> 
> Signed-off-by: Mikita Lipski 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 
> +++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index ed09e36..c572e86 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2218,6 +2218,46 @@ get_output_color_space(const struct dc_crtc_timing 
> *dc_crtc_timing)
>   return color_space;
>  }
>  
> +static void reduce_mode_colour_depth(struct dc_crtc_timing *timing_out)
> +{
> + if (timing_out->display_color_depth <= COLOR_DEPTH_888)
> + return;
> +
> + timing_out->display_color_depth--;
> +}
> +
> +static void check_if_display_depth_is_supported(struct dc_crtc_timing 
> *timing_out,

Doesn't this update timing_out->display_color_depth? We should probably name 
this something else, maybe "pick_max_supported_color_depth" or something 
similar.

Harry

> + const struct drm_display_info 
> *info)
> +{
> + int normalized_clk;
> + if (timing_out->display_color_depth <= COLOR_DEPTH_888)
> + return;
> + do {
> + normalized_clk = timing_out->pix_clk_khz;
> + /* YCbCr 4:2:0 requires additional adjustment of 1/2 */
> + if (timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420)
> + normalized_clk /= 2;
> + /* Adjusting pix clock following on HDMI spec based on colour 
> depth */
> + switch (timing_out->display_color_depth) {
> + case COLOR_DEPTH_101010:
> + normalized_clk = (normalized_clk * 30) / 24;
> + break;
> + case COLOR_DEPTH_121212:
> + normalized_clk = (normalized_clk * 36) / 24;
> + break;
> + case COLOR_DEPTH_161616:
> + normalized_clk = (normalized_clk * 48) / 24;
> + break;
> + default:
> + return;
> + }
> + if (normalized_clk <= info->max_tmds_clock)
> + return;
> + reduce_mode_colour_depth(timing_out);
> +
> + } while (timing_out->display_color_depth > COLOR_DEPTH_888);
> +
> +}
>  
> /*/
>  
>  static void
> @@ -2273,6 +2313,8 @@ fill_stream_properties_from_drm_display_mode(struct 
> dc_stream_state *stream,
>  
>   stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
>   stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
> + if (stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
> + check_if_display_depth_is_supported(timing_out, info);
>  }
>  
>  static void fill_audio_info(struct audio_info *audio_info,
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: adding ycbcr420 pixel encoding for hdmi

[Harry Wentland]

On 2018-07-04 04:38 PM, mikita.lip...@amd.com wrote:
> From: Mikita Lipski 
> 
> [why]
> HDMI EDID's VSDB contains spectial timings for specifically
> YCbCr 4:2:0 colour space. In those cases we need to verify
> if the mode provided is one of the special ones has to use
> YCbCr 4:2:0 pixel encoding for display info.
> [how]
> Verify if the mode is using specific ycbcr420 colour space with
> the help of DRM helper function and assign the mode to use
> ycbcr420 pixel encoding.
> 
> Signed-off-by: Mikita Lipski 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 01d14d8..ed09e36 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2226,7 +2226,7 @@ fill_stream_properties_from_drm_display_mode(struct 
> dc_stream_state *stream,
>const struct drm_connector 
> *connector)
>  {
>   struct dc_crtc_timing *timing_out = >timing;
> -
> + const struct drm_display_info *info = >display_info;

Nit: there should be a newline after variable declarations.

With that fixed this patch is
Reviewed-by: Harry Wentland 

Harry


>   memset(timing_out, 0, sizeof(struct dc_crtc_timing));
>  
>   timing_out->h_border_left = 0;
> @@ -2234,8 +2234,10 @@ fill_stream_properties_from_drm_display_mode(struct 
> dc_stream_state *stream,
>   timing_out->v_border_top = 0;
>   timing_out->v_border_bottom = 0;
>   /* TODO: un-hardcode */
> -
> - if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB444)
> + if (drm_mode_is_420_only(info, mode_in)
> + && stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + else if ((connector->display_info.color_formats & 
> DRM_COLOR_FORMAT_YCRCB444)
>   && stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
>   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
>   else
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/display: add a check for display depth validity

[mikita.lipski]

From: Mikita Lipski 

[why]
HDMI 2.0 fails to validate 4K@60 timing with 10 bpc
[how]
Adding a helper function that would verify if the display depth
assigned would pass a bandwidth validation.
Drop the display depth by one level till calculated pixel clk
is lower than maximum TMDS clk.

Bugzilla: https://bugs.freedesktop.org/106959

Signed-off-by: Mikita Lipski 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 +++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ed09e36..c572e86 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2218,6 +2218,46 @@ get_output_color_space(const struct dc_crtc_timing 
*dc_crtc_timing)
return color_space;
 }
 
+static void reduce_mode_colour_depth(struct dc_crtc_timing *timing_out)
+{
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+
+   timing_out->display_color_depth--;
+}
+
+static void check_if_display_depth_is_supported(struct dc_crtc_timing 
*timing_out,
+   const struct drm_display_info 
*info)
+{
+   int normalized_clk;
+   if (timing_out->display_color_depth <= COLOR_DEPTH_888)
+   return;
+   do {
+   normalized_clk = timing_out->pix_clk_khz;
+   /* YCbCr 4:2:0 requires additional adjustment of 1/2 */
+   if (timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420)
+   normalized_clk /= 2;
+   /* Adjusting pix clock following on HDMI spec based on colour 
depth */
+   switch (timing_out->display_color_depth) {
+   case COLOR_DEPTH_101010:
+   normalized_clk = (normalized_clk * 30) / 24;
+   break;
+   case COLOR_DEPTH_121212:
+   normalized_clk = (normalized_clk * 36) / 24;
+   break;
+   case COLOR_DEPTH_161616:
+   normalized_clk = (normalized_clk * 48) / 24;
+   break;
+   default:
+   return;
+   }
+   if (normalized_clk <= info->max_tmds_clock)
+   return;
+   reduce_mode_colour_depth(timing_out);
+
+   } while (timing_out->display_color_depth > COLOR_DEPTH_888);
+
+}
 /*/
 
 static void
@@ -2273,6 +2313,8 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
 
stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
+   if (stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   check_if_display_depth_is_supported(timing_out, info);
 }
 
 static void fill_audio_info(struct audio_info *audio_info,
-- 
2.7.4

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

[PATCH] drm/amd/display: adding ycbcr420 pixel encoding for hdmi

[mikita.lipski]

From: Mikita Lipski 

[why]
HDMI EDID's VSDB contains spectial timings for specifically
YCbCr 4:2:0 colour space. In those cases we need to verify
if the mode provided is one of the special ones has to use
YCbCr 4:2:0 pixel encoding for display info.
[how]
Verify if the mode is using specific ycbcr420 colour space with
the help of DRM helper function and assign the mode to use
ycbcr420 pixel encoding.

Signed-off-by: Mikita Lipski 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 01d14d8..ed09e36 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2226,7 +2226,7 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
 const struct drm_connector 
*connector)
 {
struct dc_crtc_timing *timing_out = >timing;
-
+   const struct drm_display_info *info = >display_info;
memset(timing_out, 0, sizeof(struct dc_crtc_timing));
 
timing_out->h_border_left = 0;
@@ -2234,8 +2234,10 @@ fill_stream_properties_from_drm_display_mode(struct 
dc_stream_state *stream,
timing_out->v_border_top = 0;
timing_out->v_border_bottom = 0;
/* TODO: un-hardcode */
-
-   if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB444)
+   if (drm_mode_is_420_only(info, mode_in)
+   && stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+   else if ((connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCRCB444)
&& stream->sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A)
timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
else
-- 
2.7.4

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

[PATCH v3] drm/amdgpu: update documentation for amdgpu_drv.c

[Sonny Jiang]

Signed-off-by: Sonny Jiang 
Acked-by: Junwei Zhang 
---
 Documentation/gpu/amdgpu.rst|   7 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 370 +++-
 2 files changed, 370 insertions(+), 7 deletions(-)

diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
index 765c2a3..a740e49 100644
--- a/Documentation/gpu/amdgpu.rst
+++ b/Documentation/gpu/amdgpu.rst
@@ -5,6 +5,13 @@
 The drm/amdgpu driver supports all AMD Radeon GPUs based on the Graphics Core
 Next (GCN) architecture.
 
+Module Parameters
+=
+
+The amdgpu driver supports the following module parameters:
+
+.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+
 Core Driver Infrastructure
 ==
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 963578c..72874cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1,10 +1,3 @@
-/**
- * \file amdgpu_drv.c
- * AMD Amdgpu driver
- *
- * \author Gareth Hughes 
- */
-
 /*
  * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
  * All Rights Reserved.
@@ -136,102 +129,299 @@ int amdgpu_gpu_recovery = -1; /* auto */
 int amdgpu_emu_mode = 0;
 uint amdgpu_smu_memory_pool_size = 0;
 
+/**
+ * DOC: vramlimit (int)
+ * Restrict the total amount of VRAM in MiB for testing.  The default is 0 
(Use full VRAM).
+ */
 MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
 module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
 
+/**
+ * DOC: vis_vramlimit (int)
+ * Restrict the amount of CPU visible VRAM in MiB for testing.  The default is 
0 (Use full CPU visible VRAM).
+ */
 MODULE_PARM_DESC(vis_vramlimit, "Restrict visible VRAM for testing, in 
megabytes");
 module_param_named(vis_vramlimit, amdgpu_vis_vram_limit, int, 0444);
 
+/**
+ * DOC: gartsize (uint)
+ * Restrict the size of GART in Mib (32, 64, etc.) for testing. The default is 
-1 (The size depends on asic).
+ */
 MODULE_PARM_DESC(gartsize, "Size of GART to setup in megabytes (32, 64, etc., 
-1=auto)");
 module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
 
+/**
+ * DOC: gttsize (int)
+ * Restrict the size of GTT domain in MiB for testing. The default is -1 (It's 
VRAM size if 3GB < VRAM < 3/4 RAM,
+ * otherwise 3/4 RAM size).
+ */
 MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = auto)");
 module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
 
+/**
+ * DOC: moverate (int)
+ * Set maximum buffer migration rate in MB/s. The default is -1 (8 MB/s).
+ */
 MODULE_PARM_DESC(moverate, "Maximum buffer migration rate in MB/s. (32, 64, 
etc., -1=auto, 0=1=disabled)");
 module_param_named(moverate, amdgpu_moverate, int, 0600);
 
+/**
+ * DOC: benchmark (int)
+ * Run benchmarks. The default is 0 (Skip benchmarks).
+ */
 MODULE_PARM_DESC(benchmark, "Run benchmark");
 module_param_named(benchmark, amdgpu_benchmarking, int, 0444);
 
+/**
+ * DOC: test (int)
+ * Test BO GTT->VRAM and VRAM->GTT GPU copies. The default is 0 (Skip test, 
only set 1 to run test).
+ */
 MODULE_PARM_DESC(test, "Run tests");
 module_param_named(test, amdgpu_testing, int, 0444);
 
+/**
+ * DOC: audio (int)
+ * Set Audio. The default is -1 (Enabled), set 0 to disabled it.
+ */
 MODULE_PARM_DESC(audio, "Audio enable (-1 = auto, 0 = disable, 1 = enable)");
 module_param_named(audio, amdgpu_audio, int, 0444);
 
+/**
+ * DOC: disp_priority (int)
+ * Set display Priority (0 = auto, 1 = normal, 2 = high). The default is 0.
+ */
 MODULE_PARM_DESC(disp_priority, "Display Priority (0 = auto, 1 = normal, 2 = 
high)");
 module_param_named(disp_priority, amdgpu_disp_priority, int, 0444);
 
+/**
+ * DOC: hw_i2c (int)
+ * To enable hw i2c engine. The default is 0 (Disabled).
+ */
 MODULE_PARM_DESC(hw_i2c, "hw i2c engine enable (0 = disable)");
 module_param_named(hw_i2c, amdgpu_hw_i2c, int, 0444);
 
+/**
+ * DOC: pcie_gen2 (int)
+ * To disable PCIE Gen2 mode (0 = disable, 1 = enable). The default is -1 
(auto, enabled).
+ */
 MODULE_PARM_DESC(pcie_gen2, "PCIE Gen2 mode (-1 = auto, 0 = disable, 1 = 
enable)");
 module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
 
+/**
+ * DOC: msi (int)
+ * To disable MSI functionality (1 = enable, 0 = disable). The default is -1 
(auto, enabled).
+ */
 MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
 module_param_named(msi, amdgpu_msi, int, 0444);
 
+/**
+ * DOC: lockup_timeout (int)
+ * Set GPU scheduler timeout value in ms. It must be > 0.  The default is 
1.
+ */
 MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms > 0 (default 
1)");
 module_param_named(lockup_timeout, amdgpu_lockup_timeout, int, 0444);
 
+/**
+ * DOC: dpm (int)
+ * Override for dynamic power management setting (1 = enable, 0 = disable). 
The default is -1 (auto).
+ */
 MODULE_PARM_DESC(dpm, "DPM support (1 = enable, 0 = disable, -1 = auto)");
 module_param_named(dpm, amdgpu_dpm, int, 0444);
 

Re: [PATCH] drm/amd/display/dc/dce: Fix multiple potential integer overflows

[Harry Wentland]

On 2018-07-04 01:54 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 07/04/2018 12:51 PM, Harry Wentland wrote:
> [..]
  
 @@ -145,8 +145,8 @@ static bool calculate_fb_and_fractional_fb_divider(
   * of fractional feedback decimal point and the fractional FB Divider 
 precision
   * is 2 then the equation becomes (ullfeedbackDivider + 5*100) / 
 (10*100))*/
  
 -  feedback_divider += (uint64_t)
 -  (5 * calc_pll_cs->fract_fb_divider_precision_factor);
 +  feedback_divider += 5UL *
 +  calc_pll_cs->fract_fb_divider_precision_factor;
>>>
>>> This should be 5ULL, as the commit log says, otherwise it's still only
>>> 32 bits on 32-bit platforms.
>>>
>>
>> Agreed.
>>
>> Otherwise this looks good.
>>
>> With that fixed this patch is
>> Reviewed-by: Harry Wentland 
>>
> 
> Hi Harry,
> 
> I already sent v2: https://patchwork.kernel.org/patch/10506897/
> 

Thanks. Merged.

Harry

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

Re: [PATCH] drm/amd/display: Use 2-factor allocator calls

[Harry Wentland]

On 2018-07-04 01:27 PM, Kees Cook wrote:
> As already done treewide, switch from open-coded multiplication to
> 2-factor allocation helper.
> 
> Signed-off-by: Kees Cook 

Reviewed-by: Harry Wentland 

Harry

> ---
>  drivers/gpu/drm/amd/display/modules/color/color_gamma.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c 
> b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> index 98edaefa2b47..ee69c949bfbf 100644
> --- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> +++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> @@ -1723,8 +1723,8 @@ bool  mod_color_calculate_curve(enum 
> dc_transfer_func_predefined trans,
>   kvfree(rgb_regamma);
>   } else if (trans == TRANSFER_FUNCTION_HLG ||
>   trans == TRANSFER_FUNCTION_HLG12) {
> - rgb_regamma = kvzalloc(sizeof(*rgb_regamma) *
> -(MAX_HW_POINTS + _EXTRA_POINTS),
> + rgb_regamma = kvcalloc(MAX_HW_POINTS + _EXTRA_POINTS,
> +sizeof(*rgb_regamma),
>  GFP_KERNEL);
>   if (!rgb_regamma)
>   goto rgb_regamma_alloc_fail;
> @@ -1802,8 +1802,8 @@ bool  mod_color_calculate_degamma_curve(enum 
> dc_transfer_func_predefined trans,
>   kvfree(rgb_degamma);
>   } else if (trans == TRANSFER_FUNCTION_HLG ||
>   trans == TRANSFER_FUNCTION_HLG12) {
> - rgb_degamma = kvzalloc(sizeof(*rgb_degamma) *
> -(MAX_HW_POINTS + _EXTRA_POINTS),
> + rgb_degamma = kvcalloc(MAX_HW_POINTS + _EXTRA_POINTS,
> +sizeof(*rgb_degamma),
>  GFP_KERNEL);
>   if (!rgb_degamma)
>   goto rgb_degamma_alloc_fail;
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display/dc/dce: Fix multiple potential integer overflows

[Harry Wentland]

On 2018-07-04 03:38 AM, Michel Dänzer wrote:
> On 2018-07-04 03:13 AM, Gustavo A. R. Silva wrote:
>> Add suffix ULL to constant 5 and cast variables target_pix_clk_khz and
>> feedback_divider to uint64_t in order to avoid multiple potential integer
>> overflows and give the compiler complete information about the proper
>> arithmetic to use.
>>
>> Notice that such constant and variables are used in contexts that
>> expect expressions of type uint64_t (64 bits, unsigned). The current
>> casts to uint64_t effectively apply to each expression as a whole,
>> but they do not prevent them from being evaluated using 32-bit
>> arithmetic instead of 64-bit arithmetic.
>>
>> Also, once the expressions are properly evaluated using 64-bit
>> arithmentic, there is no need for the parentheses that enclose
>> them.
>>
>> Addresses-Coverity-ID: 1460245 ("Unintentional integer overflow")
>> Addresses-Coverity-ID: 1460286 ("Unintentional integer overflow")
>> Addresses-Coverity-ID: 1460401 ("Unintentional integer overflow")
>> Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
>> Signed-off-by: Gustavo A. R. Silva 
>>
>> [...]
>>  
>> @@ -145,8 +145,8 @@ static bool calculate_fb_and_fractional_fb_divider(
>>   * of fractional feedback decimal point and the fractional FB Divider 
>> precision
>>   * is 2 then the equation becomes (ullfeedbackDivider + 5*100) / (10*100))*/
>>  
>> -feedback_divider += (uint64_t)
>> -(5 * calc_pll_cs->fract_fb_divider_precision_factor);
>> +feedback_divider += 5UL *
>> +calc_pll_cs->fract_fb_divider_precision_factor;
> 
> This should be 5ULL, as the commit log says, otherwise it's still only
> 32 bits on 32-bit platforms.
> 

Agreed.

Otherwise this looks good.

With that fixed this patch is
Reviewed-by: Harry Wentland 

Harry

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

Re: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

[Jiang, Sonny]

Hi Alex,


IP blocks indexes are not fixed. What's your idea to list them? By asic family?


enum amd_ip_block_type {
AMD_IP_BLOCK_TYPE_COMMON,
AMD_IP_BLOCK_TYPE_GMC,
AMD_IP_BLOCK_TYPE_IH,
AMD_IP_BLOCK_TYPE_SMC,
AMD_IP_BLOCK_TYPE_PSP,
AMD_IP_BLOCK_TYPE_DCE,
AMD_IP_BLOCK_TYPE_GFX,
AMD_IP_BLOCK_TYPE_SDMA,
AMD_IP_BLOCK_TYPE_UVD,
AMD_IP_BLOCK_TYPE_VCE,
AMD_IP_BLOCK_TYPE_ACP,
AMD_IP_BLOCK_TYPE_VCN
};

Thanks,

Sonny



From: Deucher, Alexander
Sent: Wednesday, July 4, 2018 2:49:17 AM
To: Qu, Jim; Zhang, Jerry; Jiang, Sonny; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c


yeah, that's a good idea.

Alex

From: amd-gfx  on behalf of Qu, Jim 

Sent: Wednesday, July 4, 2018 1:14 AM
To: Zhang, Jerry; Jiang, Sonny; amd-gfx@lists.freedesktop.org
Subject: 答复: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

I always confuse any bits definiation about some feature mask. such as 
ip_block_mask, pg_mask, cg_mask, pp_feature_mask. I think other people who is 
not familiar with amdgpu driver may have the same problem.

So, is it possible to detail every bit mask of features?

Thanks
JimQu


发件人: amd-gfx  代表 Zhang, Jerry (Junwei) 

发送时间: 2018年7月4日 12:57:01
收件人: Jiang, Sonny; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

On 07/04/2018 04:06 AM, Sonny Jiang wrote:
> Signed-off-by: Sonny Jiang 
Acked-by: Junwei Zhang 

> ---
>   Documentation/gpu/amdgpu.rst|   7 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 222 
> +++-
>   2 files changed, 222 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> index 765c2a3..a740e49 100644
> --- a/Documentation/gpu/amdgpu.rst
> +++ b/Documentation/gpu/amdgpu.rst
> @@ -5,6 +5,13 @@
>   The drm/amdgpu driver supports all AMD Radeon GPUs based on the Graphics 
> Core
>   Next (GCN) architecture.
>
> +Module Parameters
> +=
> +
> +The amdgpu driver supports the following module parameters:
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +
>   Core Driver Infrastructure
>   ==
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 963578c..caf81ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1,10 +1,3 @@
> -/**
> - * \file amdgpu_drv.c
> - * AMD Amdgpu driver
> - *
> - * \author Gareth Hughes 
> - */
> -
>   /*
>* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
>* All Rights Reserved.
> @@ -136,102 +129,235 @@ int amdgpu_gpu_recovery = -1; /* auto */
>   int amdgpu_emu_mode = 0;
>   uint amdgpu_smu_memory_pool_size = 0;
>
> +/**
> + * DOC: vramlimit (int)
> + * Restrict the total amount of VRAM in MiB for testing.  The default is 0 
> (Use full VRAM).
> + */
>   MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
>   module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>
> +/**
> + * DOC: vis_vramlimit (int)
> + * Restrict the amount of CPU visible VRAM in MiB for testing.  The default 
> is 0 (Use full CPU visible VRAM).
> + */
>   MODULE_PARM_DESC(vis_vramlimit, "Restrict visible VRAM for testing, in 
> megabytes");
>   module_param_named(vis_vramlimit, amdgpu_vis_vram_limit, int, 0444);
>
> +/**
> + * DOC: gartsize (uint)
> + * Restrict the size of GART in Mib (32, 64, etc.) for testing. The default 
> is -1 (The size depends on asic).
> + */
>   MODULE_PARM_DESC(gartsize, "Size of GART to setup in megabytes (32, 64, 
> etc., -1=auto)");
>   module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
>
> +/**
> + * DOC: gttsize (int)
> + * Restrict the size of GTT domain in MiB for testing. The default is -1 
> (It's VRAM size if 3GB < VRAM < 3/4 RAM,
> + * otherwise 3/4 RAM size).
> + */
>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = 
> auto)");
>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>
> +/**
> + * DOC: moverate (int)
> + * Set maximum buffer migration rate in MB/s. The default is -1 (8 MB/s).
> + */
>   MODULE_PARM_DESC(moverate, "Maximum buffer migration rate in MB/s. (32, 64, 
> etc., -1=auto, 0=1=disabled)");
>   module_param_named(moverate, amdgpu_moverate, int, 0600);
>
> +/**
> + * DOC: benchmark (int)
> + * Run benchmarks. The default is 0 (Skip benchmarks).
> + */
>   MODULE_PARM_DESC(benchmark, "Run benchmark");
>   module_param_named(benchmark, amdgpu_benchmarking, int, 0444);
>
> +/**
> + * DOC: test (int)
> + * Test BO GTT->VRAM and VRAM->GTT GPU copies. The default is 0 (Skip test, 
> only set 1 to run test).
> + */
>   MODULE_PARM_DESC(test, "Run tests");
>   module_param_named(test, 

[PATCH v2] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

[Michel Dänzer]

From: Michel Dänzer 

Fixes the BUG_ON spuriously triggering under the following
circumstances:

* reservation_object_reserve_shared is called with shared_count ==
  shared_max - 1, so obj->staged is freed in preparation of an in-place
  update.

* reservation_object_add_shared_fence is called with the first fence,
  after which shared_count == shared_max.

* reservation_object_add_shared_fence is called with a follow-up fence
  from the same context.

In the second reservation_object_add_shared_fence call, the BUG_ON
triggers. However, nothing bad would happen in
reservation_object_add_shared_inplace, since both fences are from the
same context, so they only occupy a single slot.

Prevent this by moving the BUG_ON to where an overflow would actually
happen (e.g. if a buggy caller didn't call
reservation_object_reserve_shared before).

v2:
* Fix description of breaking scenario (Christian König)
* Add bugzilla reference

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reviewed-by: Chris Wilson  # v1
Reviewed-by: Christian König  # v1
Signed-off-by: Michel Dänzer 
---
 drivers/dma-buf/reservation.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 314eb1071cce..532545b9488e 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -141,6 +141,7 @@ reservation_object_add_shared_inplace(struct 
reservation_object *obj,
if (signaled) {
RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
} else {
+   BUG_ON(fobj->shared_count >= fobj->shared_max);
RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
fobj->shared_count++;
}
@@ -230,10 +231,9 @@ void reservation_object_add_shared_fence(struct 
reservation_object *obj,
old = reservation_object_get_list(obj);
obj->staged = NULL;
 
-   if (!fobj) {
-   BUG_ON(old->shared_count >= old->shared_max);
+   if (!fobj)
reservation_object_add_shared_inplace(obj, old, fence);
-   } else
+   else
reservation_object_add_shared_replace(obj, old, fobj, fence);
 }
 EXPORT_SYMBOL(reservation_object_add_shared_fence);
-- 
2.18.0

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

[PATCH v2 1/2] drm/amdgpu: Add support for logging process info in amdgpu_vm.

[Andrey Grodzovsky]

Add process and thread names and pids and a function to extract
this info from relevant amdgpu_vm.

v2: Add documentation and fix identation.

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 21 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 14 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 712af5c..845f73a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2942,3 +2942,24 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, 
struct drm_file *filp)
 
return 0;
 }
+
+/**
+ * amdgpu_vm_task_info - Extracts task info for a PASID.
+ *
+ * @dev: drm device pointer
+ * @pasid: PASID identifier for VM
+ * @task_info: task_info to fill.
+ */
+void amdgpu_vm_task_info(struct amdgpu_device *adev, unsigned int pasid,
+struct amdgpu_task_info *task_info)
+{
+   struct amdgpu_vm *vm;
+
+   spin_lock(>vm_manager.pasid_lock);
+
+   vm = idr_find(>vm_manager.pasid_idr, pasid);
+   if (vm)
+   *task_info = vm->task_info;
+
+   spin_unlock(>vm_manager.pasid_lock);
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 061b99a..cd4025e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -164,6 +164,14 @@ struct amdgpu_vm_pt {
 #define AMDGPU_VM_FAULT_PASID(fault) ((u64)(fault) >> 48)
 #define AMDGPU_VM_FAULT_ADDR(fault)  ((u64)(fault) & 0xf000ULL)
 
+
+struct amdgpu_task_info {
+   charprocess_name[TASK_COMM_LEN];
+   chartask_name[TASK_COMM_LEN];
+   pid_t   pid;
+   pid_t   tgid;
+};
+
 struct amdgpu_vm {
/* tree of virtual addresses mapped */
struct rb_root_cached   va;
@@ -215,6 +223,9 @@ struct amdgpu_vm {
 
/* Valid while the PD is reserved or fenced */
uint64_tpd_phys_addr;
+
+   /* Some basic info about the task */
+   struct amdgpu_task_info task_info;
 };
 
 struct amdgpu_vm_manager {
@@ -317,4 +328,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
  struct amdgpu_job *job);
 void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
 
+void amdgpu_vm_task_info(struct amdgpu_device *adev, unsigned int pasid,
+struct amdgpu_task_info *task_info);
+
 #endif
-- 
2.7.4

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

[PATCH v2 2/2] drm/admgpu: Present amdgpu_task_info in VM_FAULTS.

[Andrey Grodzovsky]

Extract and present the reposnsible process and thread when
VM_FAULT happens.

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  | 10 --
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  |  9 +++--
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 7a625f3..1c483ea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -187,6 +187,18 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser 
*p, void *data)
if (p->uf_entry.robj)
p->job->uf_addr = uf_offset;
kfree(chunk_array);
+
+   /* Use this opportunity to fill in task info for the vm */
+   if (!vm->task_info.pid) {
+   vm->task_info.pid = current->pid;
+   get_task_comm(vm->task_info.task_name, current);
+
+   if (current->group_leader->mm == current->mm) {
+   vm->task_info.tgid = current->group_leader->pid;
+   get_task_comm(vm->task_info.process_name, 
current->group_leader);
+   }
+   }
+
return 0;
 
 free_all_kdata:
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 08753e7..7ad19f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -46,6 +46,7 @@
 
 #include "ivsrcid/ivsrcid_vislands30.h"
 
+#include "amdgpu_vm.h"
 
 static void gmc_v8_0_set_gmc_funcs(struct amdgpu_device *adev);
 static void gmc_v8_0_set_irq_funcs(struct amdgpu_device *adev);
@@ -1449,8 +1450,13 @@ static int gmc_v8_0_process_interrupt(struct 
amdgpu_device *adev,
gmc_v8_0_set_fault_enable_default(adev, false);
 
if (printk_ratelimit()) {
-   dev_err(adev->dev, "GPU fault detected: %d 0x%08x\n",
-   entry->src_id, entry->src_data[0]);
+   struct amdgpu_task_info task_info = { 0 };
+
+   amdgpu_vm_task_info(adev, entry->pasid, _info);
+
+   dev_err(adev->dev, "GPU fault detected: %d 0x%08x for process 
%s pid %d thread %s pid %d\n",
+   entry->src_id, entry->src_data[0], 
task_info.process_name,
+   task_info.tgid, task_info.task_name, task_info.pid);
dev_err(adev->dev, "  VM_CONTEXT1_PROTECTION_FAULT_ADDR   
0x%08X\n",
addr);
dev_err(adev->dev, "  VM_CONTEXT1_PROTECTION_FAULT_STATUS 
0x%08X\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 691a659..384a89c 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -259,11 +259,16 @@ static int gmc_v9_0_process_interrupt(struct 
amdgpu_device *adev,
}
 
if (printk_ratelimit()) {
+   struct amdgpu_task_info task_info = { 0 };
+
+   amdgpu_vm_task_info(adev, entry->pasid, _info);
+
dev_err(adev->dev,
-   "[%s] VMC page fault (src_id:%u ring:%u vmid:%u 
pasid:%u)\n",
+   "[%s] VMC page fault (src_id:%u ring:%u vmid:%u 
pasid:%u, for process %s pid %d thread %s pid %d\n)\n",
entry->vmid_src ? "mmhub" : "gfxhub",
entry->src_id, entry->ring_id, entry->vmid,
-   entry->pasid);
+   entry->pasid, task_info.process_name, task_info.tgid,
+   task_info.task_name, task_info.pid);
dev_err(adev->dev, "  at page 0x%016llx from %d\n",
addr, entry->client_id);
if (!amdgpu_sriov_vf(adev))
-- 
2.7.4

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

Re: [PATCH] drm/amd/display: off by one in find_irq_source_info()

[Harry Wentland]

On 2018-07-04 05:46 AM, Dan Carpenter wrote:
> The ->info[] array has DAL_IRQ_SOURCES_NUMBER elements so this condition
> should be >= instead of > or we could read one element beyond the end of
> the array.
> 
> Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
> Signed-off-by: Dan Carpenter 

Reviewed-by: Harry Wentland 

Harry

> 
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c 
> b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> index dcdfa0f01551..604bea01fc13 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> @@ -78,7 +78,7 @@ const struct irq_source_info *find_irq_source_info(
>   struct irq_service *irq_service,
>   enum dc_irq_source source)
>  {
> - if (source > DAL_IRQ_SOURCES_NUMBER || source < DC_IRQ_SOURCE_INVALID)
> + if (source >= DAL_IRQ_SOURCES_NUMBER || source < DC_IRQ_SOURCE_INVALID)
>   return NULL;
>  
>   return _service->info[source];
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: Add support for logging process info in amdgpu_vm.

[Andrey Grodzovsky]

On 07/04/2018 10:17 AM, Christian König wrote:

Am 04.07.2018 um 16:10 schrieb Andrey Grodzovsky:

Add process and thread names and pids and a function to extract
this info from relevant amdgpu_vm.

Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 14 ++
  2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

index 8370660..8ec459e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2942,3 +2942,17 @@ int amdgpu_vm_ioctl(struct drm_device *dev, 
void *data, struct drm_file *filp)

    return 0;
  }
+


Please add some sphinx documentation here.


+void amdgpu_vm_task_info(struct amdgpu_device *adev,
+  unsigned int pasid, struct amdgpu_task_info *task_info)


What editor/settings do you use?

When I trow those lines into vim's auto-formater it comes up with the 
following:


void amdgpu_vm_task_info(struct amdgpu_device *adev, unsigned int pasid,
 struct amdgpu_task_info *task_info)


I am using eclipse with CDT, I am not very into configuring the editor 
settings so not sure.
How can I use vim to vim autoformat to fix these indentation issues ? Do 
i need to install

some extra plug-in and configure it for kernel style indentation ?

Andrey



Not an issue at all, but I would like to know where that comes from 
cause it is a repeating pattern from multiple people.


Apart from that whole set looks like a nice addition to me,
Christian.


+{
+    struct amdgpu_vm *vm;
+
+    spin_lock(>vm_manager.pasid_lock);
+
+    vm = idr_find(>vm_manager.pasid_idr, pasid);
+    if (vm)
+    *task_info = vm->task_info;
+
+    spin_unlock(>vm_manager.pasid_lock);
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h

index 061b99a..88a1d18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -164,6 +164,14 @@ struct amdgpu_vm_pt {
  #define AMDGPU_VM_FAULT_PASID(fault) ((u64)(fault) >> 48)
  #define AMDGPU_VM_FAULT_ADDR(fault)  ((u64)(fault) & 
0xf000ULL)

  +
+struct amdgpu_task_info {
+    char    process_name[TASK_COMM_LEN];
+    char    task_name[TASK_COMM_LEN];
+    pid_t    pid;
+    pid_t    tgid;
+};
+
  struct amdgpu_vm {
  /* tree of virtual addresses mapped */
  struct rb_root_cached    va;
@@ -215,6 +223,9 @@ struct amdgpu_vm {
    /* Valid while the PD is reserved or fenced */
  uint64_t    pd_phys_addr;
+
+    /* Some basic info about the task */
+    struct amdgpu_task_info task_info;
  };
    struct amdgpu_vm_manager {
@@ -317,4 +328,7 @@ bool amdgpu_vm_need_pipeline_sync(struct 
amdgpu_ring *ring,

    struct amdgpu_job *job);
  void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
  +void amdgpu_vm_task_info(struct amdgpu_device *adev,
+  unsigned int pasid, struct amdgpu_task_info *task_info);
+
  #endif




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

Re: [PATCH 1/2] drm/amdgpu: Add support for logging process info in amdgpu_vm.

[Christian König]

Am 04.07.2018 um 16:10 schrieb Andrey Grodzovsky:

Add process and thread names and pids and a function to extract
this info from relevant amdgpu_vm.

Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 14 ++
  2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8370660..8ec459e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2942,3 +2942,17 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, 
struct drm_file *filp)
  
  	return 0;

  }
+


Please add some sphinx documentation here.


+void amdgpu_vm_task_info(struct amdgpu_device *adev,
+ unsigned int pasid, struct amdgpu_task_info *task_info)


What editor/settings do you use?

When I trow those lines into vim's auto-formater it comes up with the 
following:


void amdgpu_vm_task_info(struct amdgpu_device *adev, unsigned int pasid,
 struct amdgpu_task_info *task_info)

Not an issue at all, but I would like to know where that comes from 
cause it is a repeating pattern from multiple people.


Apart from that whole set looks like a nice addition to me,
Christian.


+{
+   struct amdgpu_vm *vm;
+
+   spin_lock(>vm_manager.pasid_lock);
+
+   vm = idr_find(>vm_manager.pasid_idr, pasid);
+   if (vm)
+   *task_info = vm->task_info;
+
+   spin_unlock(>vm_manager.pasid_lock);
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 061b99a..88a1d18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -164,6 +164,14 @@ struct amdgpu_vm_pt {
  #define AMDGPU_VM_FAULT_PASID(fault) ((u64)(fault) >> 48)
  #define AMDGPU_VM_FAULT_ADDR(fault)  ((u64)(fault) & 0xf000ULL)
  
+

+struct amdgpu_task_info {
+   charprocess_name[TASK_COMM_LEN];
+   chartask_name[TASK_COMM_LEN];
+   pid_t   pid;
+   pid_t   tgid;
+};
+
  struct amdgpu_vm {
/* tree of virtual addresses mapped */
struct rb_root_cached   va;
@@ -215,6 +223,9 @@ struct amdgpu_vm {
  
  	/* Valid while the PD is reserved or fenced */

uint64_tpd_phys_addr;
+
+   /* Some basic info about the task */
+   struct amdgpu_task_info task_info;
  };
  
  struct amdgpu_vm_manager {

@@ -317,4 +328,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
  struct amdgpu_job *job);
  void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
  
+void amdgpu_vm_task_info(struct amdgpu_device *adev,

+ unsigned int pasid, struct amdgpu_task_info *task_info);
+
  #endif


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

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Michel Dänzer]

On 2018-07-04 09:57 AM, Zhang, Jerry (Junwei) wrote:
> On 07/04/2018 03:21 PM, Michel Dänzer wrote:
>> On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:
>>> On 07/04/2018 02:34 PM, Christian König wrote:
 Am 04.07.2018 um 05:02 schrieb Junwei Zhang:
> From: Michel Dänzer 
>
> Without this, there could not be enough slots, which could trigger the
> BUG_ON in reservation_object_add_shared_fence.
>
> v2:
> * Jump to the error label instead of returning directly (Jerry Zhang)
> v3:
> * Reserve slots for command submission after VM updates (Christian
> König)
>
> Cc: sta...@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/106418
> Reported-by: mikhail.v.gavri...@gmail.com
> Signed-off-by: Michel Dänzer 
> Signed-off-by: Junwei Zhang 

 I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
 only a minor nit pick.
>>>
>>> At first, I really put it at the end of amdgpu_bo_vm_update_pte().
>>> On the 2nd thought, that func may be called by others(although it's not
>>> for now), so I move it out of there to the caller.
>>
>> Makes sense to me, FWIW. Thanks Junwei and Christian!
> 
> Please confirm if that works in your side.
> If yes, I would push that to drm-next then.

Go ahead, seems fine.


P.S. Maybe the shortlog could be more specific, e.g.

drm/amdgpu: Reserve VM root shared fence slot for command submission

-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: separate gpu address from bo pin

[Christian König]

Am 26.06.2018 um 10:35 schrieb Junwei Zhang:

It could be got by amdgpu_bo_gpu_offset() if need

Signed-off-by: Junwei Zhang 
Reviewed-by: Michel Dänzer 


Patches #1 and #3 are Reviewed-by: Christian König 
.


Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c|  6 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c |  6 --
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c|  5 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  5 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c|  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c  |  6 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c| 17 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h|  5 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_test.c  |  6 --
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  2 +-
  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 10 +-
  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 10 +-
  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 10 +-
  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 10 +-
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 ++-
  17 files changed, 50 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 305143f..98e3bf8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -251,7 +251,6 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
struct amdgpu_bo *bo = NULL;
struct amdgpu_bo_param bp;
int r;
-   uint64_t gpu_addr_tmp = 0;
void *cpu_ptr_tmp = NULL;
  
  	memset(, 0, sizeof(bp));

@@ -275,8 +274,7 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
goto allocate_mem_reserve_bo_failed;
}
  
-	r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT,

-   _addr_tmp);
+   r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
if (r) {
dev_err(adev->dev, "(%d) failed to pin bo for amdkfd\n", r);
goto allocate_mem_pin_bo_failed;
@@ -290,7 +288,7 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
}
  
  	*mem_obj = bo;

-   *gpu_addr = gpu_addr_tmp;
+   *gpu_addr = amdgpu_bo_gpu_offset(bo);
*cpu_ptr = cpu_ptr_tmp;
  
  	amdgpu_bo_unreserve(bo);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index ff8fd75..079af8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1587,7 +1587,7 @@ int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct 
kgd_dev *kgd,
goto bo_reserve_failed;
}
  
-	ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);

+   ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
if (ret) {
pr_err("Failed to pin bo. ret %d\n", ret);
goto pin_failed;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
index 19cfff3..cb88d7e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
@@ -95,7 +95,8 @@ static void amdgpu_benchmark_move(struct amdgpu_device *adev, 
unsigned size,
r = amdgpu_bo_reserve(sobj, false);
if (unlikely(r != 0))
goto out_cleanup;
-   r = amdgpu_bo_pin(sobj, sdomain, );
+   r = amdgpu_bo_pin(sobj, sdomain);
+   saddr = amdgpu_bo_gpu_offset(sobj);
amdgpu_bo_unreserve(sobj);
if (r) {
goto out_cleanup;
@@ -108,7 +109,8 @@ static void amdgpu_benchmark_move(struct amdgpu_device 
*adev, unsigned size,
r = amdgpu_bo_reserve(dobj, false);
if (unlikely(r != 0))
goto out_cleanup;
-   r = amdgpu_bo_pin(dobj, ddomain, );
+   r = amdgpu_bo_pin(dobj, ddomain);
+   daddr = amdgpu_bo_gpu_offset(dobj);
amdgpu_bo_unreserve(dobj);
if (r) {
goto out_cleanup;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 995dc02..d2745de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2736,11 +2736,10 @@ int amdgpu_device_resume(struct drm_device *dev, bool 
resume, bool fbcon)
struct amdgpu_bo *aobj = 
gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
r = amdgpu_bo_reserve(aobj, true);
if (r == 0) {
-   r = amdgpu_bo_pin(aobj,
- AMDGPU_GEM_DOMAIN_VRAM,
- _crtc->cursor_addr);
+   

Re: [PATCH] drm/amdgpu: allocate gart memory when it's required (v3)

[Christian König]

Am 27.06.2018 um 10:28 schrieb Junwei Zhang:

Instead of calling gart memory on every bo pin,
allocates it on demand

v2: fix error handling
v3: drop the change for kfd gtt bo mapping, not needed.

Signed-off-by: Junwei Zhang 


Reviewed-by: Christian König 


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c|  6 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c | 14 --
  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  6 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c|  8 
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c| 15 +--
  drivers/gpu/drm/amd/amdgpu/amdgpu_test.c  |  5 +
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++--
  7 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 98e3bf8..e3ed08d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -280,6 +280,12 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
goto allocate_mem_pin_bo_failed;
}
  
+	r = amdgpu_ttm_alloc_gart(>tbo);

+   if (r) {
+   dev_err(adev->dev, "%p bind failed\n", bo);
+   goto allocate_mem_kmap_bo_failed;
+   }
+
r = amdgpu_bo_kmap(bo, _ptr_tmp);
if (r) {
dev_err(adev->dev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
index cb88d7e..3079ea8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
@@ -96,11 +96,16 @@ static void amdgpu_benchmark_move(struct amdgpu_device 
*adev, unsigned size,
if (unlikely(r != 0))
goto out_cleanup;
r = amdgpu_bo_pin(sobj, sdomain);
-   saddr = amdgpu_bo_gpu_offset(sobj);
+   if (r) {
+   amdgpu_bo_unreserve(sobj);
+   goto out_cleanup;
+   }
+   r = amdgpu_ttm_alloc_gart(>tbo);
amdgpu_bo_unreserve(sobj);
if (r) {
goto out_cleanup;
}
+   saddr = amdgpu_bo_gpu_offset(sobj);
bp.domain = ddomain;
r = amdgpu_bo_create(adev, , );
if (r) {
@@ -110,11 +115,16 @@ static void amdgpu_benchmark_move(struct amdgpu_device 
*adev, unsigned size,
if (unlikely(r != 0))
goto out_cleanup;
r = amdgpu_bo_pin(dobj, ddomain);
-   daddr = amdgpu_bo_gpu_offset(dobj);
+   if (r) {
+   amdgpu_bo_unreserve(sobj);
+   goto out_cleanup;
+   }
+   r = amdgpu_ttm_alloc_gart(>tbo);
amdgpu_bo_unreserve(dobj);
if (r) {
goto out_cleanup;
}
+   daddr = amdgpu_bo_gpu_offset(dobj);
  
  	if (adev->mman.buffer_funcs) {

time = amdgpu_benchmark_do_move(adev, size, saddr, daddr, n);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 036b6f7..7d6a36b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -194,6 +194,12 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc 
*crtc,
goto unreserve;
}
  
+	r = amdgpu_ttm_alloc_gart(_abo->tbo);

+   if (unlikely(r != 0)) {
+   DRM_ERROR("%p bind failed\n", new_abo);
+   goto unpin;
+   }
+
r = reservation_object_get_fences_rcu(new_abo->tbo.resv, >excl,
  >shared_count,
  >shared);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index 462b7a1..cd68a2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -173,6 +173,14 @@ static int amdgpufb_create_pinned_object(struct 
amdgpu_fbdev *rfbdev,
amdgpu_bo_unreserve(abo);
goto out_unref;
}
+
+   ret = amdgpu_ttm_alloc_gart(>tbo);
+   if (ret) {
+   amdgpu_bo_unreserve(abo);
+   dev_err(adev->dev, "%p bind failed\n", abo);
+   goto out_unref;
+   }
+
ret = amdgpu_bo_kmap(abo, NULL);
amdgpu_bo_unreserve(abo);
if (ret) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 79cdbf1..7f7c221 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -257,6 +257,13 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
dev_err(adev->dev, "(%d) kernel bo pin failed\n", r);
goto error_unreserve;
}
+
+   r = amdgpu_ttm_alloc_gart(&(*bo_ptr)->tbo);
+   if (r) {
+   dev_err(adev->dev, "%p bind failed\n", *bo_ptr);
+   goto error_unpin;
+   }
+
if (gpu_addr)
   

Re: [PATCH] drm/amdgpu: simplify the bo reference on amdgpu_vo_update

[Christian König]

Am 04.07.2018 um 12:16 schrieb Huang Rui:

BO ptr already be initialized at definition, we needn't use the complicated
reference.

Signed-off-by: Huang Rui 


There is a typo in the subject line, with that fixed the patch is 
Reviewed-by: Christian König .


Regards,
Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 712af5c..b53562b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1645,18 +1645,17 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
uint64_t flags;
int r;
  
-	if (clear || !bo_va->base.bo) {

+   if (clear || !bo) {
mem = NULL;
nodes = NULL;
exclusive = NULL;
} else {
struct ttm_dma_tt *ttm;
  
-		mem = _va->base.bo->tbo.mem;

+   mem = >tbo.mem;
nodes = mem->mm_node;
if (mem->mem_type == TTM_PL_TT) {
-   ttm = container_of(bo_va->base.bo->tbo.ttm,
-  struct ttm_dma_tt, ttm);
+   ttm = container_of(bo->tbo.ttm, struct ttm_dma_tt, ttm);
pages_addr = ttm->dma_address;
}
exclusive = reservation_object_get_excl(bo->tbo.resv);


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

[PATCH] drm/amdgpu: simplify the bo reference on amdgpu_vo_update

[Huang Rui]

BO ptr already be initialized at definition, we needn't use the complicated
reference.

Signed-off-by: Huang Rui 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 712af5c..b53562b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1645,18 +1645,17 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
uint64_t flags;
int r;
 
-   if (clear || !bo_va->base.bo) {
+   if (clear || !bo) {
mem = NULL;
nodes = NULL;
exclusive = NULL;
} else {
struct ttm_dma_tt *ttm;
 
-   mem = _va->base.bo->tbo.mem;
+   mem = >tbo.mem;
nodes = mem->mm_node;
if (mem->mem_type == TTM_PL_TT) {
-   ttm = container_of(bo_va->base.bo->tbo.ttm,
-  struct ttm_dma_tt, ttm);
+   ttm = container_of(bo->tbo.ttm, struct ttm_dma_tt, ttm);
pages_addr = ttm->dma_address;
}
exclusive = reservation_object_get_excl(bo->tbo.resv);
-- 
2.7.4

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

[PATCH] drm/amd/display: off by one in find_irq_source_info()

[Dan Carpenter]

The ->info[] array has DAL_IRQ_SOURCES_NUMBER elements so this condition
should be >= instead of > or we could read one element beyond the end of
the array.

Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c 
b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
index dcdfa0f01551..604bea01fc13 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
@@ -78,7 +78,7 @@ const struct irq_source_info *find_irq_source_info(
struct irq_service *irq_service,
enum dc_irq_source source)
 {
-   if (source > DAL_IRQ_SOURCES_NUMBER || source < DC_IRQ_SOURCE_INVALID)
+   if (source >= DAL_IRQ_SOURCES_NUMBER || source < DC_IRQ_SOURCE_INVALID)
return NULL;
 
return _service->info[source];
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

[Christian König]

Am 04.07.2018 um 11:09 schrieb Michel Dänzer:

On 2018-07-04 10:31 AM, Christian König wrote:

Am 26.06.2018 um 16:31 schrieb Michel Dänzer:

From: Michel Dänzer 

Fixes the BUG_ON spuriously triggering under the following
circumstances:

* ttm_eu_reserve_buffers processes a list containing multiple BOs using
    the same reservation object, so it calls
    reservation_object_reserve_shared with that reservation object once
    for each such BO.
* In reservation_object_reserve_shared, old->shared_count ==
    old->shared_max - 1, so obj->staged is freed in preparation of an
    in-place update.
* ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence
    once for each of the BOs above, always with the same fence.
* The first call adds the fence in the remaining free slot, after which
    old->shared_count == old->shared_max.

Well, the explanation here is not correct. For multiple BOs using the
same reservation object we won't call
reservation_object_add_shared_fence() multiple times because we move
those to the duplicates list in ttm_eu_reserve_buffers().

But this bug can still happen because we call
reservation_object_add_shared_fence() manually with fences for the same
context in a couple of places.

One prominent case which comes to my mind are for the VM BOs during
updates. Another possibility are VRAM BOs which need to be cleared.

Thanks. How about the following:

* ttm_eu_reserve_buffers calls reservation_object_reserve_shared.
* In reservation_object_reserve_shared, shared_count == shared_max - 1,
   so obj->staged is freed in preparation of an in-place update.
* ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence,
   after which shared_count == shared_max.
* The amdgpu driver also calls reservation_object_add_shared_fence for
   the same reservation object, and the BUG_ON triggers.


I would rather completely drop the reference to the ttm_eu_* functions, 
cause those wrappers are completely unrelated to the problem.


Instead let's just note something like the following:

* When reservation_object_reserve_shared is called with shared_count == 
shared_max - 1,

  so obj->staged is freed in preparation of an in-place update.

* Now reservation_object_add_shared_fence is called with the first fence 
and after that shared_count == shared_max.


* After that  reservation_object_add_shared_fence can be called with 
follow up fences from the same context, but since shared_count == 
shared_max we would run into this BUG_ON.



However, nothing bad would happen in
reservation_object_add_shared_inplace, since all fences use the same
context, so they can only occupy a single slot.

Prevent this by moving the BUG_ON to where an overflow would actually
happen (e.g. if a buggy caller didn't call
reservation_object_reserve_shared before).


Also, I'll add a reference to https://bugs.freedesktop.org/106418 in v2,
as I suspect this fix is necessary under the circumstances described
there as well.


The rest sounds good to me.

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

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Zhang, Jerry (Junwei)]

On 07/04/2018 05:15 PM, Michel Dänzer wrote:

On 2018-07-04 11:01 AM, Christian König wrote:

Am 04.07.2018 um 09:57 schrieb Zhang, Jerry (Junwei):

On 07/04/2018 03:21 PM, Michel Dänzer wrote:

On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:

On 07/04/2018 02:34 PM, Christian König wrote:

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger
the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian
König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 


I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
only a minor nit pick.


At first, I really put it at the end of amdgpu_bo_vm_update_pte().
On the 2nd thought, that func may be called by others(although it's not
for now), so I move it out of there to the caller.


Well that is exactly the reason I wanted to have it in
amdgpu_bo_vm_update_pte() :)

But you are right, there are good arguments for both approach and as
long as we don't have a second user it doesn't matter.


In general, I think it's better to keep the
reservation_object_reserve_shared calls as close as possible to the
corresponding reservation_object_add_shared_fence calls, otherwise it's
hard to keep track of whether or not a slot is reserved at any given point.


Agree, I also consider that previously, just before the command submission.
while I think 2 more things then:

1) amdgpu_cs_ib_vm_chunk() is unique func during cs ioctl,
in another word, it's a MUST part of that, so it will not impact others.
Additionally it also checks if vm used, otherwise, the reserve is not needed 
either.

2) adding the reserve with vm checking in amdgpu_cs_submit() looks not so good 
as well.
IMO. so I decide to insert that in amdgpu_cs_ib_vm_chunk() vm existing case.

BTW, we may add some comments in code as well to better reference in the future.

Jerry





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

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Michel Dänzer]

On 2018-07-04 11:01 AM, Christian König wrote:
> Am 04.07.2018 um 09:57 schrieb Zhang, Jerry (Junwei):
>> On 07/04/2018 03:21 PM, Michel Dänzer wrote:
>>> On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:
 On 07/04/2018 02:34 PM, Christian König wrote:
> Am 04.07.2018 um 05:02 schrieb Junwei Zhang:
>> From: Michel Dänzer 
>>
>> Without this, there could not be enough slots, which could trigger
>> the
>> BUG_ON in reservation_object_add_shared_fence.
>>
>> v2:
>> * Jump to the error label instead of returning directly (Jerry Zhang)
>> v3:
>> * Reserve slots for command submission after VM updates (Christian
>> König)
>>
>> Cc: sta...@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/106418
>> Reported-by: mikhail.v.gavri...@gmail.com
>> Signed-off-by: Michel Dänzer 
>> Signed-off-by: Junwei Zhang 
>
> I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
> only a minor nit pick.

 At first, I really put it at the end of amdgpu_bo_vm_update_pte().
 On the 2nd thought, that func may be called by others(although it's not
 for now), so I move it out of there to the caller.
> 
> Well that is exactly the reason I wanted to have it in
> amdgpu_bo_vm_update_pte() :)
> 
> But you are right, there are good arguments for both approach and as
> long as we don't have a second user it doesn't matter.

In general, I think it's better to keep the
reservation_object_reserve_shared calls as close as possible to the
corresponding reservation_object_add_shared_fence calls, otherwise it's
hard to keep track of whether or not a slot is reserved at any given point.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

[Michel Dänzer]

On 2018-07-04 10:31 AM, Christian König wrote:
> Am 26.06.2018 um 16:31 schrieb Michel Dänzer:
>> From: Michel Dänzer 
>>
>> Fixes the BUG_ON spuriously triggering under the following
>> circumstances:
>>
>> * ttm_eu_reserve_buffers processes a list containing multiple BOs using
>>    the same reservation object, so it calls
>>    reservation_object_reserve_shared with that reservation object once
>>    for each such BO.
>> * In reservation_object_reserve_shared, old->shared_count ==
>>    old->shared_max - 1, so obj->staged is freed in preparation of an
>>    in-place update.
>> * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence
>>    once for each of the BOs above, always with the same fence.
>> * The first call adds the fence in the remaining free slot, after which
>>    old->shared_count == old->shared_max.
> 
> Well, the explanation here is not correct. For multiple BOs using the
> same reservation object we won't call
> reservation_object_add_shared_fence() multiple times because we move
> those to the duplicates list in ttm_eu_reserve_buffers().
> 
> But this bug can still happen because we call
> reservation_object_add_shared_fence() manually with fences for the same
> context in a couple of places.
> 
> One prominent case which comes to my mind are for the VM BOs during
> updates. Another possibility are VRAM BOs which need to be cleared.

Thanks. How about the following:

* ttm_eu_reserve_buffers calls reservation_object_reserve_shared.
* In reservation_object_reserve_shared, shared_count == shared_max - 1,
  so obj->staged is freed in preparation of an in-place update.
* ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence,
  after which shared_count == shared_max.
* The amdgpu driver also calls reservation_object_add_shared_fence for
  the same reservation object, and the BUG_ON triggers.

However, nothing bad would happen in
reservation_object_add_shared_inplace, since all fences use the same
context, so they can only occupy a single slot.

Prevent this by moving the BUG_ON to where an overflow would actually
happen (e.g. if a buggy caller didn't call
reservation_object_reserve_shared before).


Also, I'll add a reference to https://bugs.freedesktop.org/106418 in v2,
as I suspect this fix is necessary under the circumstances described
there as well.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Christian König]

Am 04.07.2018 um 09:21 schrieb Michel Dänzer:

On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:

On 07/04/2018 02:34 PM, Christian König wrote:

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian
König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 

I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
only a minor nit pick.

At first, I really put it at the end of amdgpu_bo_vm_update_pte().
On the 2nd thought, that func may be called by others(although it's not
for now), so I move it out of there to the caller.

Makes sense to me, FWIW. Thanks Junwei and Christian!


P.S. Christian, any feedback on
https://patchwork.freedesktop.org/patch/232204/ ?


Just did, thanks for pointing that out once more.

Read the mail during my vacation, but forgot to answer it on Monday.

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

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Christian König]

Am 04.07.2018 um 09:57 schrieb Zhang, Jerry (Junwei):

On 07/04/2018 03:21 PM, Michel Dänzer wrote:

On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:

On 07/04/2018 02:34 PM, Christian König wrote:

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger 
the

BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian
König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 


I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
only a minor nit pick.


At first, I really put it at the end of amdgpu_bo_vm_update_pte().
On the 2nd thought, that func may be called by others(although it's not
for now), so I move it out of there to the caller.


Well that is exactly the reason I wanted to have it in 
amdgpu_bo_vm_update_pte() :)


But you are right, there are good arguments for both approach and as 
long as we don't have a second user it doesn't matter.


Christian.



Makes sense to me, FWIW. Thanks Junwei and Christian!


Please confirm if that works in your side.
If yes, I would push that to drm-next then.

Thanks.

Jerry




P.S. Christian, any feedback on
https://patchwork.freedesktop.org/patch/232204/ ?



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

Re: [PATCH] drm/amdgpu/acp: Fix slab-out-of-bounds in mfd_add_device in acp_hw_init

[Mukunda,Vijendar]

On Tuesday 03 July 2018 09:50 PM, Alex Deucher wrote:

On Mon, Jul 2, 2018 at 5:48 PM, Daniel Kurtz  wrote:

Hi Alex,

On Sun, Apr 15, 2018 at 9:48 PM Agrawal, Akshu  wrote:




On 4/13/2018 9:45 PM, Daniel Kurtz wrote:

Commit 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for
stoney/cz") added support for the "BT_I2S" ACP i2s channel.  As part of
this change, one additional acp resource was added, but the "num_resource"
count was accidentally incremented by 2.

This incorrect count eventually causes mfd_add_device() to try to access
an invalid memory address (the location of non-existent resource 5.

This fault was detected by running a KASAN enabled kernel, which produced
the following splat at boot:

[6.612987] 
==
[6.613509] BUG: KASAN: slab-out-of-bounds in mfd_add_device+0x4bc/0x7a7
[6.613509] Read of size 8 at addr 880107d4dc58 by task swapper/0/1
[6.613509]
[6.613509] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.33 #349
[6.613509] Hardware name: Google Grunt/Grunt, BIOS 
Google_Grunt.10543.0.2018_04_03_1812 04/02/2018
[6.613509] Call Trace:
[6.613509]  dump_stack+0x4d/0x63
[6.613509]  print_address_description+0x80/0x2d6
[6.613509]  ? mfd_add_device+0x4bc/0x7a7
[6.613509]  kasan_report+0x255/0x295
[6.613509]  mfd_add_device+0x4bc/0x7a7
[6.613509]  ? kasan_kmalloc+0x99/0xa8
[6.613509]  ? mfd_add_devices+0x58/0xe4
[6.613509]  ? __kmalloc+0x154/0x178
[6.613509]  mfd_add_devices+0xa5/0xe4
[6.613509]  acp_hw_init+0x92e/0xc4a
[6.613509]  amdgpu_device_init+0x1dfb/0x22a2
[6.613509]  ? kmalloc_order+0x53/0x5d
[6.613509]  ? kmalloc_order_trace+0x23/0xb3
[6.613509]  amdgpu_driver_load_kms+0xce/0x267
[6.613509]  drm_dev_register+0x169/0x2fb
[6.613509]  amdgpu_pci_probe+0x217/0x242
[6.613509]  pci_device_probe+0x101/0x18e
[6.613509]  driver_probe_device+0x1dd/0x419
[6.613509]  ? ___might_sleep+0x80/0x1b6
[6.613509]  __driver_attach+0x9f/0xc9
[6.613509]  ? driver_probe_device+0x419/0x419
[6.613509]  bus_for_each_dev+0xbc/0xe1
[6.613509]  bus_add_driver+0x189/0x2c0
[6.613509]  driver_register+0x108/0x156
[6.613509]  ? ttm_init+0x67/0x67
[6.613509]  do_one_initcall+0xb2/0x161
[6.613509]  kernel_init_freeable+0x25a/0x308
[6.613509]  ? rest_init+0xcc/0xcc
[6.613509]  kernel_init+0x11/0x10d
[6.613509]  ? rest_init+0xcc/0xcc
[6.613509]  ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Allocated by task 1:
[6.613509]  save_stack+0x46/0xce
[6.613509]  kasan_kmalloc+0x99/0xa8
[6.613509]  kmem_cache_alloc_trace+0x11a/0x13e
[6.613509]  acp_hw_init+0x210/0xc4a
[6.613509]  amdgpu_device_init+0x1dfb/0x22a2
[6.613509]  amdgpu_driver_load_kms+0xce/0x267
[6.613509]  drm_dev_register+0x169/0x2fb
[6.613509]  amdgpu_pci_probe+0x217/0x242
[6.613509]  pci_device_probe+0x101/0x18e
[6.613509]  driver_probe_device+0x1dd/0x419
[6.613509]  __driver_attach+0x9f/0xc9
[6.613509]  bus_for_each_dev+0xbc/0xe1
[6.613509]  bus_add_driver+0x189/0x2c0
[6.613509]  driver_register+0x108/0x156
[6.613509]  do_one_initcall+0xb2/0x161
[6.613509]  kernel_init_freeable+0x25a/0x308
[6.613509]  kernel_init+0x11/0x10d
[6.613509]  ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Freed by task 0:
[6.613509] (stack is not available)
[6.613509]
[6.613509] The buggy address belongs to the object at 880107d4db08
[6.613509]  which belongs to the cache kmalloc-512 of size 512
[6.613509] The buggy address is located 336 bytes inside of
[6.613509]  512-byte region [880107d4db08, 880107d4dd08)
[6.613509] The buggy address belongs to the page:
[6.613509] page:ea00041f5300 count:1 mapcount:0 mapping:  
(null) index:0x0 compound_mapcount: 0
[6.613509] flags: 0x80008100(slab|head)
[6.613509] raw: 80008100   
000100120012
[6.613509] raw: ea0004208520 88010b001680 88010b002cc0 

[6.613509] page dumped because: kasan: bad access detected
[6.613509]
[6.613509] Memory state around the buggy address:
[6.613509]  880107d4db00: fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[6.613509]  880107d4db80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[6.613509] >880107d4dc00: 00 00 00 00 00 00 00 00 00 fc fc fc fc fc fc 
fc
[6.613509] ^
[6.613509]  880107d4dc80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[6.613509]  880107d4dd00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[6.613509] 
==

Fixes: 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for stoney/cz")
Signed-off-by: Daniel Kurtz 

Acked-by: Akshu Agrawal 



Was this patch 

Re: [PATCH 0/5] drm: use core pcie functionality for pcie gen/width

[Christian König]

Whole series is Acked-by: Christian König .

BTW: With patch #2 you created quite some noise in the news:

https://www.tomshardware.com/news/amd-vega-20-pcie-4.0,37389.html

Cheers,
Christian.

Am 25.06.2018 um 23:06 schrieb Alex Deucher:

This series exports some pcie helper functions for use by drivers and
fixes up the amdgpu and radeon drivers to use this core functionality
rather than the duplicated functionality in the drm.  Finally we remove
the drm helpers since the duplicate the pcie functionality of the core.
This also adds proper pcie gen4 detection for amdgpu.

Alex Deucher (5):
   pci: export pcie_get_speed_cap and pcie_get_width_cap
   drm/amdgpu: update amd_pcie.h to include gen4 speeds
   drm/amdgpu: use pcie functions for link width and speed
   drm/radeon: use pcie functions for link width
   drm: drop drm_pcie_get_speed_cap_mask and drm_pcie_get_max_link_width

  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 83 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.c|  7 ++-
  drivers/gpu/drm/amd/amdgpu/ci_dpm.c|  3 +-
  drivers/gpu/drm/amd/amdgpu/si_dpm.c|  3 +-
  drivers/gpu/drm/amd/include/amd_pcie.h |  2 +
  drivers/gpu/drm/drm_pci.c  | 58 -
  drivers/gpu/drm/radeon/ci_dpm.c| 20 +--
  drivers/gpu/drm/radeon/cik.c   | 22 
  drivers/gpu/drm/radeon/r600_dpm.c  |  4 +-
  drivers/gpu/drm/radeon/radeon.h|  4 ++
  drivers/gpu/drm/radeon/si.c| 22 
  drivers/gpu/drm/radeon/si_dpm.c| 20 +--
  drivers/pci/pci.c  |  2 +
  include/drm/drm_pci.h  |  7 ---
  include/linux/pci.h|  3 ++
  15 files changed, 132 insertions(+), 128 deletions(-)



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

Re: [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

[Christian König]

Am 26.06.2018 um 16:31 schrieb Michel Dänzer:

From: Michel Dänzer 

Fixes the BUG_ON spuriously triggering under the following
circumstances:

* ttm_eu_reserve_buffers processes a list containing multiple BOs using
   the same reservation object, so it calls
   reservation_object_reserve_shared with that reservation object once
   for each such BO.
* In reservation_object_reserve_shared, old->shared_count ==
   old->shared_max - 1, so obj->staged is freed in preparation of an
   in-place update.
* ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence
   once for each of the BOs above, always with the same fence.
* The first call adds the fence in the remaining free slot, after which
   old->shared_count == old->shared_max.


Well, the explanation here is not correct. For multiple BOs using the 
same reservation object we won't call 
reservation_object_add_shared_fence() multiple times because we move 
those to the duplicates list in ttm_eu_reserve_buffers().


But this bug can still happen because we call 
reservation_object_add_shared_fence() manually with fences for the same 
context in a couple of places.


One prominent case which comes to my mind are for the VM BOs during 
updates. Another possibility are VRAM BOs which need to be cleared.




In the next call to reservation_object_add_shared_fence, the BUG_ON
triggers. However, nothing bad would happen in
reservation_object_add_shared_inplace, since the fence is already in the
reservation object.

Prevent this by moving the BUG_ON to where an overflow would actually
happen (e.g. if a buggy caller didn't call
reservation_object_reserve_shared before).

Cc: sta...@vger.kernel.org
Signed-off-by: Michel Dänzer 


Reviewed-by: Christian König .

Regards,
Christian.


---
  drivers/dma-buf/reservation.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 314eb1071cce..532545b9488e 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -141,6 +141,7 @@ reservation_object_add_shared_inplace(struct 
reservation_object *obj,
if (signaled) {
RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
} else {
+   BUG_ON(fobj->shared_count >= fobj->shared_max);
RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
fobj->shared_count++;
}
@@ -230,10 +231,9 @@ void reservation_object_add_shared_fence(struct 
reservation_object *obj,
old = reservation_object_get_list(obj);
obj->staged = NULL;
  
-	if (!fobj) {

-   BUG_ON(old->shared_count >= old->shared_max);
+   if (!fobj)
reservation_object_add_shared_inplace(obj, old, fence);
-   } else
+   else
reservation_object_add_shared_replace(obj, old, fobj, fence);
  }
  EXPORT_SYMBOL(reservation_object_add_shared_fence);


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

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Zhang, Jerry (Junwei)]

On 07/04/2018 03:21 PM, Michel Dänzer wrote:

On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:

On 07/04/2018 02:34 PM, Christian König wrote:

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian
König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 


I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
only a minor nit pick.


At first, I really put it at the end of amdgpu_bo_vm_update_pte().
On the 2nd thought, that func may be called by others(although it's not
for now), so I move it out of there to the caller.


Makes sense to me, FWIW. Thanks Junwei and Christian!


Please confirm if that works in your side.
If yes, I would push that to drm-next then.

Thanks.

Jerry




P.S. Christian, any feedback on
https://patchwork.freedesktop.org/patch/232204/ ?


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

Re: [PATCH] drm/amd/display/dc/dce: Fix multiple potential integer overflows

[Michel Dänzer]

On 2018-07-04 03:13 AM, Gustavo A. R. Silva wrote:
> Add suffix ULL to constant 5 and cast variables target_pix_clk_khz and
> feedback_divider to uint64_t in order to avoid multiple potential integer
> overflows and give the compiler complete information about the proper
> arithmetic to use.
> 
> Notice that such constant and variables are used in contexts that
> expect expressions of type uint64_t (64 bits, unsigned). The current
> casts to uint64_t effectively apply to each expression as a whole,
> but they do not prevent them from being evaluated using 32-bit
> arithmetic instead of 64-bit arithmetic.
> 
> Also, once the expressions are properly evaluated using 64-bit
> arithmentic, there is no need for the parentheses that enclose
> them.
> 
> Addresses-Coverity-ID: 1460245 ("Unintentional integer overflow")
> Addresses-Coverity-ID: 1460286 ("Unintentional integer overflow")
> Addresses-Coverity-ID: 1460401 ("Unintentional integer overflow")
> Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
> Signed-off-by: Gustavo A. R. Silva 
> 
> [...]
>  
> @@ -145,8 +145,8 @@ static bool calculate_fb_and_fractional_fb_divider(
>   * of fractional feedback decimal point and the fractional FB Divider 
> precision
>   * is 2 then the equation becomes (ullfeedbackDivider + 5*100) / (10*100))*/
>  
> - feedback_divider += (uint64_t)
> - (5 * calc_pll_cs->fract_fb_divider_precision_factor);
> + feedback_divider += 5UL *
> + calc_pll_cs->fract_fb_divider_precision_factor;

This should be 5ULL, as the commit log says, otherwise it's still only
32 bits on 32-bit platforms.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Michel Dänzer]

On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:
> On 07/04/2018 02:34 PM, Christian König wrote:
>> Am 04.07.2018 um 05:02 schrieb Junwei Zhang:
>>> From: Michel Dänzer 
>>>
>>> Without this, there could not be enough slots, which could trigger the
>>> BUG_ON in reservation_object_add_shared_fence.
>>>
>>> v2:
>>> * Jump to the error label instead of returning directly (Jerry Zhang)
>>> v3:
>>> * Reserve slots for command submission after VM updates (Christian
>>> König)
>>>
>>> Cc: sta...@vger.kernel.org
>>> Bugzilla: https://bugs.freedesktop.org/106418
>>> Reported-by: mikhail.v.gavri...@gmail.com
>>> Signed-off-by: Michel Dänzer 
>>> Signed-off-by: Junwei Zhang 
>>
>> I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
>> only a minor nit pick.
> 
> At first, I really put it at the end of amdgpu_bo_vm_update_pte().
> On the 2nd thought, that func may be called by others(although it's not
> for now), so I move it out of there to the caller.

Makes sense to me, FWIW. Thanks Junwei and Christian!


P.S. Christian, any feedback on
https://patchwork.freedesktop.org/patch/232204/ ?

-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/5] drm/amdgpu: update amd_pcie.h to include gen4 speeds

[Alex Deucher]

On Mon, Jun 25, 2018 at 5:06 PM, Alex Deucher  wrote:
> Internal header used by the driver to specify pcie gen
> speeds of the asic and chipset.
>
> Signed-off-by: Alex Deucher 

Anyone care to review patches 2,3,4?

> ---
>  drivers/gpu/drm/amd/include/amd_pcie.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/include/amd_pcie.h 
> b/drivers/gpu/drm/amd/include/amd_pcie.h
> index 5eb895fd98bf..9cb9ceb4d74d 100644
> --- a/drivers/gpu/drm/amd/include/amd_pcie.h
> +++ b/drivers/gpu/drm/amd/include/amd_pcie.h
> @@ -27,6 +27,7 @@
>  #define CAIL_PCIE_LINK_SPEED_SUPPORT_GEN10x0001
>  #define CAIL_PCIE_LINK_SPEED_SUPPORT_GEN20x0002
>  #define CAIL_PCIE_LINK_SPEED_SUPPORT_GEN30x0004
> +#define CAIL_PCIE_LINK_SPEED_SUPPORT_GEN40x0008
>  #define CAIL_PCIE_LINK_SPEED_SUPPORT_MASK0x
>  #define CAIL_PCIE_LINK_SPEED_SUPPORT_SHIFT   16
>
> @@ -34,6 +35,7 @@
>  #define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN1   0x0001
>  #define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN2   0x0002
>  #define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN3   0x0004
> +#define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN4   0x0008
>  #define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_MASK   0x
>  #define CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_SHIFT  0
>
> --
> 2.13.6
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Zhang, Jerry (Junwei)]

On 07/04/2018 02:34 PM, Christian König wrote:

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 


I would put that at the end of amdgpu_bo_vm_update_pte(), but that is only a 
minor nit pick.


At first, I really put it at the end of amdgpu_bo_vm_update_pte().
On the 2nd thought, that func may be called by others(although it's not for 
now),
so I move it out of there to the caller.

Jerry



Patch is Reviewed-by: Christian König  anyway.

Regards,
Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 7a625f3..1bc0281 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -928,6 +928,10 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device 
*adev,
  r = amdgpu_bo_vm_update_pte(p);
  if (r)
  return r;
+
+r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv);
+if (r)
+return r;
  }
  return amdgpu_cs_sync_rings(p);



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

Re: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

[Deucher, Alexander]

yeah, that's a good idea.

Alex

From: amd-gfx  on behalf of Qu, Jim 

Sent: Wednesday, July 4, 2018 1:14 AM
To: Zhang, Jerry; Jiang, Sonny; amd-gfx@lists.freedesktop.org
Subject: 答复: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

I always confuse any bits definiation about some feature mask. such as 
ip_block_mask, pg_mask, cg_mask, pp_feature_mask. I think other people who is 
not familiar with amdgpu driver may have the same problem.

So, is it possible to detail every bit mask of features?

Thanks
JimQu


发件人: amd-gfx  代表 Zhang, Jerry (Junwei) 

发送时间: 2018年7月4日 12:57:01
收件人: Jiang, Sonny; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH v2] drm/amdgpu: update documentation for amdgpu_drv.c

On 07/04/2018 04:06 AM, Sonny Jiang wrote:
> Signed-off-by: Sonny Jiang 
Acked-by: Junwei Zhang 

> ---
>   Documentation/gpu/amdgpu.rst|   7 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 222 
> +++-
>   2 files changed, 222 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> index 765c2a3..a740e49 100644
> --- a/Documentation/gpu/amdgpu.rst
> +++ b/Documentation/gpu/amdgpu.rst
> @@ -5,6 +5,13 @@
>   The drm/amdgpu driver supports all AMD Radeon GPUs based on the Graphics 
> Core
>   Next (GCN) architecture.
>
> +Module Parameters
> +=
> +
> +The amdgpu driver supports the following module parameters:
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +
>   Core Driver Infrastructure
>   ==
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 963578c..caf81ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1,10 +1,3 @@
> -/**
> - * \file amdgpu_drv.c
> - * AMD Amdgpu driver
> - *
> - * \author Gareth Hughes 
> - */
> -
>   /*
>* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
>* All Rights Reserved.
> @@ -136,102 +129,235 @@ int amdgpu_gpu_recovery = -1; /* auto */
>   int amdgpu_emu_mode = 0;
>   uint amdgpu_smu_memory_pool_size = 0;
>
> +/**
> + * DOC: vramlimit (int)
> + * Restrict the total amount of VRAM in MiB for testing.  The default is 0 
> (Use full VRAM).
> + */
>   MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
>   module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>
> +/**
> + * DOC: vis_vramlimit (int)
> + * Restrict the amount of CPU visible VRAM in MiB for testing.  The default 
> is 0 (Use full CPU visible VRAM).
> + */
>   MODULE_PARM_DESC(vis_vramlimit, "Restrict visible VRAM for testing, in 
> megabytes");
>   module_param_named(vis_vramlimit, amdgpu_vis_vram_limit, int, 0444);
>
> +/**
> + * DOC: gartsize (uint)
> + * Restrict the size of GART in Mib (32, 64, etc.) for testing. The default 
> is -1 (The size depends on asic).
> + */
>   MODULE_PARM_DESC(gartsize, "Size of GART to setup in megabytes (32, 64, 
> etc., -1=auto)");
>   module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
>
> +/**
> + * DOC: gttsize (int)
> + * Restrict the size of GTT domain in MiB for testing. The default is -1 
> (It's VRAM size if 3GB < VRAM < 3/4 RAM,
> + * otherwise 3/4 RAM size).
> + */
>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = 
> auto)");
>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>
> +/**
> + * DOC: moverate (int)
> + * Set maximum buffer migration rate in MB/s. The default is -1 (8 MB/s).
> + */
>   MODULE_PARM_DESC(moverate, "Maximum buffer migration rate in MB/s. (32, 64, 
> etc., -1=auto, 0=1=disabled)");
>   module_param_named(moverate, amdgpu_moverate, int, 0600);
>
> +/**
> + * DOC: benchmark (int)
> + * Run benchmarks. The default is 0 (Skip benchmarks).
> + */
>   MODULE_PARM_DESC(benchmark, "Run benchmark");
>   module_param_named(benchmark, amdgpu_benchmarking, int, 0444);
>
> +/**
> + * DOC: test (int)
> + * Test BO GTT->VRAM and VRAM->GTT GPU copies. The default is 0 (Skip test, 
> only set 1 to run test).
> + */
>   MODULE_PARM_DESC(test, "Run tests");
>   module_param_named(test, amdgpu_testing, int, 0444);
>
> +/**
> + * DOC: audio (int)
> + * Set Audio. The default is -1 (Enabled), set 0 to disabled it.
> + */
>   MODULE_PARM_DESC(audio, "Audio enable (-1 = auto, 0 = disable, 1 = 
> enable)");
>   module_param_named(audio, amdgpu_audio, int, 0444);
>
> +/**
> + * DOC: disp_priority (int)
> + * Set display Priority (0 = auto, 1 = normal, 2 = high). The default is 0.
> + */
>   MODULE_PARM_DESC(disp_priority, "Display Priority (0 = auto, 1 = normal, 2 
> = high)");
>   module_param_named(disp_priority, amdgpu_disp_priority, int, 0444);
>
> +/**
> + * DOC: hw_i2c (int)
> + * To enable hw i2c engine. The default is 0 (Disabled).
> + */
>   MODULE_PARM_DESC(hw_i2c, "hw i2c engine enable (0 = disable)");
>   module_param_named(hw_i2c, 

Re: [PATCH] drm/amdgpu: Reserve fence slots for command submission

[Christian König]

Am 04.07.2018 um 05:02 schrieb Junwei Zhang:

From: Michel Dänzer 

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)
v3:
* Reserve slots for command submission after VM updates (Christian König)

Cc: sta...@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavri...@gmail.com
Signed-off-by: Michel Dänzer 
Signed-off-by: Junwei Zhang 


I would put that at the end of amdgpu_bo_vm_update_pte(), but that is 
only a minor nit pick.


Patch is Reviewed-by: Christian König  anyway.

Regards,
Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 7a625f3..1bc0281 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -928,6 +928,10 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device 
*adev,
r = amdgpu_bo_vm_update_pte(p);
if (r)
return r;
+
+   r = 
reservation_object_reserve_shared(vm->root.base.bo->tbo.resv);
+   if (r)
+   return r;
}
  
  	return amdgpu_cs_sync_rings(p);


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

[PATCH] drm/amd/display/dc/dce: Fix multiple potential integer overflows

[Gustavo A. R. Silva]

Add suffix ULL to constant 5 and cast variables target_pix_clk_khz and
feedback_divider to uint64_t in order to avoid multiple potential integer
overflows and give the compiler complete information about the proper
arithmetic to use.

Notice that such constant and variables are used in contexts that
expect expressions of type uint64_t (64 bits, unsigned). The current
casts to uint64_t effectively apply to each expression as a whole,
but they do not prevent them from being evaluated using 32-bit
arithmetic instead of 64-bit arithmetic.

Also, once the expressions are properly evaluated using 64-bit
arithmentic, there is no need for the parentheses that enclose
them.

Addresses-Coverity-ID: 1460245 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1460286 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1460401 ("Unintentional integer overflow")
Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c 
b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
index 88b09dd..715d737 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
@@ -133,7 +133,7 @@ static bool calculate_fb_and_fractional_fb_divider(
uint64_t feedback_divider;
 
feedback_divider =
-   (uint64_t)(target_pix_clk_khz * ref_divider * post_divider);
+   (uint64_t)target_pix_clk_khz * ref_divider * post_divider;
feedback_divider *= 10;
/* additional factor, since we divide by 10 afterwards */
feedback_divider *= (uint64_t)(calc_pll_cs->fract_fb_divider_factor);
@@ -145,8 +145,8 @@ static bool calculate_fb_and_fractional_fb_divider(
  * of fractional feedback decimal point and the fractional FB Divider precision
  * is 2 then the equation becomes (ullfeedbackDivider + 5*100) / (10*100))*/
 
-   feedback_divider += (uint64_t)
-   (5 * calc_pll_cs->fract_fb_divider_precision_factor);
+   feedback_divider += 5UL *
+   calc_pll_cs->fract_fb_divider_precision_factor;
feedback_divider =
div_u64(feedback_divider,
calc_pll_cs->fract_fb_divider_precision_factor * 10);
@@ -203,8 +203,8 @@ static bool calc_fb_divider_checking_tolerance(
_feedback_divider);
 
/*Actual calculated value*/
-   actual_calc_clk_khz = (uint64_t)(feedback_divider *
-   calc_pll_cs->fract_fb_divider_factor) +
+   actual_calc_clk_khz = (uint64_t)feedback_divider *
+   calc_pll_cs->fract_fb_divider_factor +
fract_feedback_divider;
actual_calc_clk_khz *= calc_pll_cs->ref_freq_khz;
actual_calc_clk_khz =
-- 
2.7.4

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