Re: [PATCH] drm/amdgpu: stream's id should reduced after stream destruct

[Kazlauskas, Nicholas]

On 2021-02-20 1:30 a.m., ZhiJie.Zhang wrote:

Signed-off-by: ZhiJie.Zhang 
---
  drivers/gpu/drm/amd/display/dc/core/dc_stream.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index c103f858375d..dc7b7e57a86c 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -137,6 +137,8 @@ static void dc_stream_destruct(struct dc_stream_state 
*stream)
dc_transfer_func_release(stream->out_transfer_func);
stream->out_transfer_func = NULL;
}
+
+   stream->ctx->dc_stream_id_count--;


This is supposed to be a unique identifier so we shouldn't be reusing 
any old ID when creating a new stream.


Regards,
Nicholas Kazlauskas


  }
  
  void dc_stream_retain(struct dc_stream_state *stream)

Re: [PATCH AUTOSEL 5.4 006/130] drm/amd/display: Do not silently accept DCC for multiplane formats.

[Kazlauskas, Nicholas]

On 2020-12-29 9:54 a.m., Deucher, Alexander wrote:

[AMD Public Use]


I don't know if these fixes related to modifiers make sense in the 
pre-modifier code base.  Bas, Nick?


Alex


Mesa should be the only userspace trying to make use of DCC and it 
doesn't do it for video formats. From the kernel side of things we've 
also never supported this and you'd get corruption on the screen if you 
tried.


It's a "fix" for both pre-modifiers and post-modifiers code.

Regards,
Nicholas Kazlauskas



*From:* amd-gfx  on behalf of 
Sasha Levin 

*Sent:* Tuesday, December 22, 2020 9:16 PM
*To:* linux-kernel@vger.kernel.org ; 
sta...@vger.kernel.org 
*Cc:* Sasha Levin ; dri-de...@lists.freedesktop.org 
; amd-...@lists.freedesktop.org 
; Bas Nieuwenhuizen 
; Deucher, Alexander 
; Kazlauskas, Nicholas 

*Subject:* [PATCH AUTOSEL 5.4 006/130] drm/amd/display: Do not silently 
accept DCC for multiplane formats.

From: Bas Nieuwenhuizen 

[ Upstream commit b35ce7b364ec80b54f48a8fdf9fb74667774d2da ]

Silently accepting it could result in corruption.

Signed-off-by: Bas Nieuwenhuizen 
Reviewed-by: Alex Deucher 
Reviewed-by: Nicholas Kazlauskas 
Signed-off-by: Alex Deucher 
Signed-off-by: Sasha Levin 
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

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 d2dd387c95d86..ce70c42a2c3ec 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2734,7 +2734,7 @@ fill_plane_dcc_attributes(struct amdgpu_device *adev,
  return 0;

  if (format >= SURFACE_PIXEL_FORMAT_VIDEO_BEGIN)
-   return 0;
+   return -EINVAL;

  if (!dc->cap_funcs.get_dcc_compression_cap)
  return -EINVAL;
--
2.27.0

___
amd-gfx mailing list
amd-...@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Calexander.deucher%40amd.com%7Cfb9f2581393f494acd1708d8a6e905fc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63744286704415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ZYz1FjTl6SoWX1B91t0McdUai%2FzRF9C8uBmE%2BNQNod4%3Dreserved=0 
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Calexander.deucher%40amd.com%7Cfb9f2581393f494acd1708d8a6e905fc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63744286704415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ZYz1FjTl6SoWX1B91t0McdUai%2FzRF9C8uBmE%2BNQNod4%3Dreserved=0>

Re: [PATCH 0/3] drm/amd/display: Fix kernel panic by breakpoint

[Kazlauskas, Nicholas]

Reviewed-by: Nicholas Kazlauskas 

Looks fine to me. Feel free to apply.

Regards,
Nicholas Kazlauskas

On 2020-10-26 3:34 p.m., Alex Deucher wrote:

Yes, looks good to me as well.  Series is:
Acked-by: Alex Deucher 
I'll give the display guys a few more days to look this over, but if
there are no objections, I'll apply them.

Thanks!

Alex

On Fri, Oct 23, 2020 at 7:16 PM Luben Tuikov  wrote:


On 2020-10-23 03:46, Takashi Iwai wrote:

Hi,

the amdgpu driver's ASSERT_CRITICAL() macro calls the
kgdb_breakpoing() even if no debug option is set, and this leads to a
kernel panic on distro kernels.  The first two patches are the
oneliner fixes for those, while the last one is the cleanup of those
debug macros.


This looks like good work and solid. Hopefully it gets picked up.

Regards,
Luben




Takashi

===

Takashi Iwai (3):
   drm/amd/display: Fix kernel panic by dal_gpio_open() error
   drm/amd/display: Don't invoke kgdb_breakpoint() unconditionally
   drm/amd/display: Clean up debug macros

  drivers/gpu/drm/amd/display/Kconfig |  1 +
  drivers/gpu/drm/amd/display/dc/gpio/gpio_base.c |  4 +--
  drivers/gpu/drm/amd/display/dc/os_types.h   | 33 +
  3 files changed, 15 insertions(+), 23 deletions(-)



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

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

Re: [PATCH v2 0/4] Enlarge tracepoints in the display component

[Kazlauskas, Nicholas]

On 2020-09-16 5:12 a.m., Daniel Vetter wrote:

On Fri, Sep 11, 2020 at 10:59:23AM -0400, Rodrigo Siqueira wrote:

Debug issues related to display can be a challenge due to the complexity
around this topic and different source of information might help in this
process. We already have support for tracepoints inside the display
component, i.e., we have the basic functionalities available and we just
need to expand it in order to make it more valuable for debugging. For
this reason, this patchset reworks part of the current tracepoint
options and add different sets of tracing inside amdgpu_dm, display
core, and DCN10. The first patch of this series just rework part of the
current tracepoints and the last set of patches introduces new
tracepoints.

This first patchset version is functional. Please, let me know what I
can improve in the current version but also let me know what kind of
tracepoint I can add for the next version.

Finally, I want to highlight that this work is based on a set of patches
originally made by Nicholas Kazlauskas.

Change in V2:
- I added another patch for capturing the clock state for different display
   architecture.


Hm I'm not super sure tracepoints for state dumping are the right thing
here. We kinda have the atomic state dumping code with all the various
callbacks, and you can extend that pretty easily. Gives you full state
dump in debugfs, plus a few function to dump into dmesg.

Maybe what we need is a function to dump this also into printk tracepoint
(otoh with Sean Paul's tracepoint work we'd get that through the dmesg
stuff already), and then you could do it there?

Upside is that for customers they'd get a much more consistent way to
debug display issues across different drivers.

For low-level hw debug what we do is give the hw guys an mmio trace, and
they replay it on the fancy boxes :-) So for that I think this here is
again too high level, but maybe what you have is a bit different.
-Daniel


We have raw register traces, but what I find most useful is to be able 
to see are the incoming DRM IOCTLs, objects and properties per commit.


Many of the bugs we see in display code is in the conversion from DRM -> 
DM -> DC state. The current HW state is kind of useless in most cases, 
but the sequence helps track down intermittent problems and understand 
state transitions.


Tracepoints provide everything I really need to be able to track down 
these problems without falling back to a full debugger. The existing DRM 
prints (even at high logging levels) aren't enough to understand what's 
going on in most cases in our driver so funneling those into tracepoints 
to improve perf doesn't really help that much.


I think this kind of idea was rejected for DRM core last year with 
Sean's patch series but if we can't get them into core then I'd like to 
get them into our driver at least. These are a cleaned up version of 
Sean's work + my work that I end up applying locally whenever I debug 
something.


Regards,
Nicholas Kazlauskas





Rodrigo Siqueira (4):
   drm/amd/display: Rework registers tracepoint
   drm/amd/display: Add tracepoint for amdgpu_dm
   drm/amd/display: Add pipe_state tracepoint
   drm/amd/display: Add tracepoint for capturing clocks state

  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  17 +
  .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 712 +-
  .../dc/clk_mgr/dce112/dce112_clk_mgr.c|   5 +
  .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c|   4 +
  .../display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c  |   4 +
  .../amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c |   4 +
  .../display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c  |   4 +
  drivers/gpu/drm/amd/display/dc/core/dc.c  |  11 +
  .../gpu/drm/amd/display/dc/dce/dce_clk_mgr.c  |   5 +
  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  17 +-
  10 files changed, 747 insertions(+), 36 deletions(-)

--
2.28.0

Re: [PATCH v2 1/4] drm/amd/display: Rework registers tracepoint

[Kazlauskas, Nicholas]

On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:

amdgpu_dc_rreg and amdgpu_dc_wreg are very similar, for this reason,
this commits abstract these two events by using DECLARE_EVENT_CLASS and
create an instance of it for each one of these events.

Signed-off-by: Rodrigo Siqueira 


This looks reasonable to me. Does this still show up as 
amdpgu_dc_rrreg/amdgpu_dc_wreg in the captured trace log?


As long as we can still tell this apart you can consider this patch:

Reviewed-by: Nicholas Kazlauskas 

Regards,
Nicholas Kazlauskas


---
  .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 55 ---
  1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index d898981684d5..dd34e11b1079 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -31,40 +31,33 @@
  
  #include 
  
-TRACE_EVENT(amdgpu_dc_rreg,

-   TP_PROTO(unsigned long *read_count, uint32_t reg, uint32_t value),
-   TP_ARGS(read_count, reg, value),
-   TP_STRUCT__entry(
-   __field(uint32_t, reg)
-   __field(uint32_t, value)
-   ),
-   TP_fast_assign(
-   __entry->reg = reg;
-   __entry->value = value;
-   *read_count = *read_count + 1;
-   ),
-   TP_printk("reg=0x%08lx, value=0x%08lx",
-   (unsigned long)__entry->reg,
-   (unsigned long)__entry->value)
-);
+DECLARE_EVENT_CLASS(amdgpu_dc_reg_template,
+   TP_PROTO(unsigned long *count, uint32_t reg, uint32_t 
value),
+   TP_ARGS(count, reg, value),
  
-TRACE_EVENT(amdgpu_dc_wreg,

-   TP_PROTO(unsigned long *write_count, uint32_t reg, uint32_t value),
-   TP_ARGS(write_count, reg, value),
-   TP_STRUCT__entry(
-   __field(uint32_t, reg)
-   __field(uint32_t, value)
-   ),
-   TP_fast_assign(
-   __entry->reg = reg;
-   __entry->value = value;
-   *write_count = *write_count + 1;
-   ),
-   TP_printk("reg=0x%08lx, value=0x%08lx",
-   (unsigned long)__entry->reg,
-   (unsigned long)__entry->value)
+   TP_STRUCT__entry(
+__field(uint32_t, reg)
+__field(uint32_t, value)
+   ),
+
+   TP_fast_assign(
+  __entry->reg = reg;
+  __entry->value = value;
+  *count = *count + 1;
+   ),
+
+   TP_printk("reg=0x%08lx, value=0x%08lx",
+ (unsigned long)__entry->reg,
+ (unsigned long)__entry->value)
  );
  
+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_rreg,

+TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),
+TP_ARGS(count, reg, value));
+
+DEFINE_EVENT(amdgpu_dc_reg_template, amdgpu_dc_wreg,
+TP_PROTO(unsigned long *count, uint32_t reg, uint32_t value),
+TP_ARGS(count, reg, value));
  
  TRACE_EVENT(amdgpu_dc_performance,

TP_PROTO(unsigned long read_count, unsigned long write_count,

Re: [PATCH v2 3/4] drm/amd/display: Add pipe_state tracepoint

[Kazlauskas, Nicholas]

On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:

This commit introduces a trace mechanism for struct pipe_ctx by adding a
middle layer struct in the amdgpu_dm_trace.h for capturing the most
important data from struct pipe_ctx and showing its data via tracepoint.
This tracepoint was added to dc.c and dcn10_hw_sequencer, however, it
can be added to other DCN architecture.

Co-developed-by: Nicholas Kazlauskas 
Signed-off-by: Nicholas Kazlauskas 
Signed-off-by: Rodrigo Siqueira 



This patch, while very useful, unfortunately pulls in a lot of DM code 
into DC so I would prefer to hold off on this one for now.


It would be better if this had a proper DC interface for tracing/logging 
these states. If the API was more like how we handle tracing register 
reads/writes this would be cleaner.


Regards,
Nicholas Kazlauskas


---
  .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 172 ++
  drivers/gpu/drm/amd/display/dc/core/dc.c  |  11 ++
  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  17 +-
  3 files changed, 195 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index 5fb4c4a5c349..53f62506e17c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -376,6 +376,178 @@ TRACE_EVENT(amdgpu_dm_atomic_check_finish,
  __entry->async_update, __entry->allow_modeset)
  );
  
+#ifndef _AMDGPU_DM_TRACE_STRUCTS_DEFINED_

+#define _AMDGPU_DM_TRACE_STRUCTS_DEFINED_
+
+struct amdgpu_dm_trace_pipe_state {
+   int pipe_idx;
+   const void *stream;
+   int stream_w;
+   int stream_h;
+   int dst_x;
+   int dst_y;
+   int dst_w;
+   int dst_h;
+   int src_x;
+   int src_y;
+   int src_w;
+   int src_h;
+   int clip_x;
+   int clip_y;
+   int clip_w;
+   int clip_h;
+   int recout_x;
+   int recout_y;
+   int recout_w;
+   int recout_h;
+   int viewport_x;
+   int viewport_y;
+   int viewport_w;
+   int viewport_h;
+   int flip_immediate;
+   int surface_pitch;
+   int format;
+   int swizzle;
+   unsigned int update_flags;
+};
+
+#define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \
+   do { \
+   trace_pipe_state.pipe_idx   = (pipe_ctx)->pipe_idx; \
+   trace_pipe_state.stream = (pipe_ctx)->stream; \
+   trace_pipe_state.stream_w   = 
(pipe_ctx)->stream->timing.h_addressable; \
+   trace_pipe_state.stream_h   = 
(pipe_ctx)->stream->timing.v_addressable; \
+   trace_pipe_state.dst_x  = 
(pipe_ctx)->plane_state->dst_rect.x; \
+   trace_pipe_state.dst_y  = 
(pipe_ctx)->plane_state->dst_rect.y; \
+   trace_pipe_state.dst_w  = 
(pipe_ctx)->plane_state->dst_rect.width; \
+   trace_pipe_state.dst_h  = 
(pipe_ctx)->plane_state->dst_rect.height; \
+   trace_pipe_state.src_x  = 
(pipe_ctx)->plane_state->src_rect.x; \
+   trace_pipe_state.src_y  = 
(pipe_ctx)->plane_state->src_rect.y; \
+   trace_pipe_state.src_w  = 
(pipe_ctx)->plane_state->src_rect.width; \
+   trace_pipe_state.src_h  = 
(pipe_ctx)->plane_state->src_rect.height; \
+   trace_pipe_state.clip_x = 
(pipe_ctx)->plane_state->clip_rect.x; \
+   trace_pipe_state.clip_y = 
(pipe_ctx)->plane_state->clip_rect.y; \
+   trace_pipe_state.clip_w = 
(pipe_ctx)->plane_state->clip_rect.width; \
+   trace_pipe_state.clip_h = 
(pipe_ctx)->plane_state->clip_rect.height; \
+   trace_pipe_state.recout_x   = 
(pipe_ctx)->plane_res.scl_data.recout.x; \
+   trace_pipe_state.recout_y   = 
(pipe_ctx)->plane_res.scl_data.recout.y; \
+   trace_pipe_state.recout_w   = 
(pipe_ctx)->plane_res.scl_data.recout.width; \
+   trace_pipe_state.recout_h   = 
(pipe_ctx)->plane_res.scl_data.recout.height; \
+   trace_pipe_state.viewport_x = 
(pipe_ctx)->plane_res.scl_data.viewport.x; \
+   trace_pipe_state.viewport_y = 
(pipe_ctx)->plane_res.scl_data.viewport.y; \
+   trace_pipe_state.viewport_w = 
(pipe_ctx)->plane_res.scl_data.viewport.width; \
+   trace_pipe_state.viewport_h = 
(pipe_ctx)->plane_res.scl_data.viewport.height; \
+   trace_pipe_state.flip_immediate = 
(pipe_ctx)->plane_state->flip_immediate; \
+   trace_pipe_state.surface_pitch  = 
(pipe_ctx)->plane_state->plane_size.surface_pitch; \
+   trace_pipe_state.format = 
(pipe_ctx)->plane_state->format; \
+   trace_pipe_state.swizzle= 
(pipe_ctx)->plane_state->tiling_info.gfx9.swizzle; \
+   

Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

No objections from my side - and thanks for addressing my feedback.

Regards,
Nicholas Kazlauskas

On 2020-08-18 12:15 p.m., Alex Deucher wrote:

Applied.  Thanks!

Alex

On Mon, Aug 17, 2020 at 1:59 PM Alex Deucher  wrote:


On Mon, Aug 17, 2020 at 3:09 AM Alexander Monakov  wrote:


Ping.


Patch looks good to me:
Reviewed-by: Alex Deucher 

Nick, unless you have any objections, I'll go ahead and apply it.

Alex



On Tue, 4 Aug 2020, Alexander Monakov wrote:


Documentation for sysfs backlight level interface requires that
values in both 'brightness' and 'actual_brightness' files are
interpreted to be in range from 0 to the value given in the
'max_brightness' file.

With amdgpu, max_brightness gives 255, and values written by the user
into 'brightness' are internally rescaled to a wider range. However,
reading from 'actual_brightness' gives the raw register value without
inverse rescaling. This causes issues for various userspace tools such
as PowerTop and systemd that expect the value to be in the correct
range.

Introduce a helper to retrieve internal backlight range. Use it to
reimplement 'convert_brightness' as 'convert_brightness_from_user' and
introduce 'convert_brightness_to_user'.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242
Cc: Alex Deucher 
Cc: Nicholas Kazlauskas 
Signed-off-by: Alexander Monakov 
---
v2: split convert_brightness to &_from_user and &_to_user (Nicholas)

  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +--
  1 file changed, 40 insertions(+), 41 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 710edc70e37e..b60a763f3f95 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2881,51 +2881,50 @@ static int set_backlight_via_aux(struct dc_link *link, 
uint32_t brightness)
   return rc ? 0 : 1;
  }

-static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,
-   const uint32_t user_brightness)
+static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
+ unsigned *min, unsigned *max)
  {
- u32 min, max, conversion_pace;
- u32 brightness = user_brightness;
-
   if (!caps)
- goto out;
+ return 0;

- if (!caps->aux_support) {
- max = caps->max_input_signal;
- min = caps->min_input_signal;
- /*
-  * The brightness input is in the range 0-255
-  * It needs to be rescaled to be between the
-  * requested min and max input signal
-  * It also needs to be scaled up by 0x101 to
-  * match the DC interface which has a range of
-  * 0 to 0x
-  */
- conversion_pace = 0x101;
- brightness =
- user_brightness
- * conversion_pace
- * (max - min)
- / AMDGPU_MAX_BL_LEVEL
- + min * conversion_pace;
+ if (caps->aux_support) {
+ // Firmware limits are in nits, DC API wants millinits.
+ *max = 1000 * caps->aux_max_input_signal;
+ *min = 1000 * caps->aux_min_input_signal;
   } else {
- /* TODO
-  * We are doing a linear interpolation here, which is OK but
-  * does not provide the optimal result. We probably want
-  * something close to the Perceptual Quantizer (PQ) curve.
-  */
- max = caps->aux_max_input_signal;
- min = caps->aux_min_input_signal;
-
- brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min
-+ user_brightness * max;
- // Multiple the value by 1000 since we use millinits
- brightness *= 1000;
- brightness = DIV_ROUND_CLOSEST(brightness, AMDGPU_MAX_BL_LEVEL);
+ // Firmware limits are 8-bit, PWM control is 16-bit.
+ *max = 0x101 * caps->max_input_signal;
+ *min = 0x101 * caps->min_input_signal;
   }
+ return 1;
+}

-out:
- return brightness;
+static u32 convert_brightness_from_user(const struct amdgpu_dm_backlight_caps 
*caps,
+ uint32_t brightness)
+{
+ unsigned min, max;
+
+ if (!get_brightness_range(caps, , ))
+ return brightness;
+
+ // Rescale 0..255 to min..max
+ return min + DIV_ROUND_CLOSEST((max - min) * brightness,
+AMDGPU_MAX_BL_LEVEL);
+}
+
+static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps 
*caps,
+   uint32_t brightness)
+{
+ unsigned min, max;
+
+ if (!get_brightness_range(caps, , ))
+ return brightness;
+
+ if (brightness < min)
+   

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

On 2020-08-04 12:28 p.m., Alexander Monakov wrote:



On Tue, 4 Aug 2020, Kazlauskas, Nicholas wrote:


This is a cleaner change the other proposed patch since it doesn't need to


Can you give a URL to the other patch please?


Sorry, replied to the wrong email by accident here.

The other change was modifying the max_brightness range and rescaling 
internal min/max defaults.


I don't think it was sent out to the list yet.

Regards,
Nicholas Kazlauskas




modify the exist conversion functions but I'd be worried about broken
userspace relying on 0-255 as the only acceptable range.


Not sure what you mean by this. Userspace simply reads the maximum value from
max_brightness sysfs file. On other gpu/firmware combinations it can be 7 or 9
for example, it just happens to be 255 with modern amdgpu. Minimum value is
always zero.

Value seen in max_brightness remains 255 with this patch, so as far as userspace
is concerned nothing is changed apart from value given by actual_brightness 
file.

Alexander



Not an expert on existing implementations to point out a specific one though.

Regards,
Nicholas Kazlauskas

On 2020-08-03 4:02 p.m., Alexander Monakov wrote:

Documentation for sysfs backlight level interface requires that
values in both 'brightness' and 'actual_brightness' files are
interpreted to be in range from 0 to the value given in the
'max_brightness' file.

With amdgpu, max_brightness gives 255, and values written by the user
into 'brightness' are internally rescaled to a wider range. However,
reading from 'actual_brightness' gives the raw register value without
inverse rescaling. This causes issues for various userspace tools such
as PowerTop and systemd that expect the value to be in the correct
range.

Introduce a helper to retrieve internal backlight range. Extend the
existing 'convert_brightness' function to handle conversion in both
directions.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242
Cc: Alex Deucher 
Signed-off-by: Alexander Monakov 
---
   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 73 ---
   1 file changed, 32 insertions(+), 41 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 710edc70e37e..03e21e7b7917 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2881,51 +2881,42 @@ static int set_backlight_via_aux(struct dc_link
*link, uint32_t brightness)
return rc ? 0 : 1;
   }
   -static u32 convert_brightness(const struct amdgpu_dm_backlight_caps
*caps,
- const uint32_t user_brightness)
+static int get_brightness_range(const struct amdgpu_dm_backlight_caps
*caps,
+   unsigned *min, unsigned *max)
   {
-   u32 min, max, conversion_pace;
-   u32 brightness = user_brightness;
-
if (!caps)
-   goto out;
+   return 0;
   -if (!caps->aux_support) {
-   max = caps->max_input_signal;
-   min = caps->min_input_signal;
-   /*
-* The brightness input is in the range 0-255
-* It needs to be rescaled to be between the
-* requested min and max input signal
-* It also needs to be scaled up by 0x101 to
-* match the DC interface which has a range of
-* 0 to 0x
-*/
-   conversion_pace = 0x101;
-   brightness =
-   user_brightness
-   * conversion_pace
-   * (max - min)
-   / AMDGPU_MAX_BL_LEVEL
-   + min * conversion_pace;
+   if (caps->aux_support) {
+   // Firmware limits are in nits, DC API wants millinits.
+   *max = 1000 * caps->aux_max_input_signal;
+   *min = 1000 * caps->aux_min_input_signal;
} else {
-   /* TODO
-* We are doing a linear interpolation here, which is OK but
-* does not provide the optimal result. We probably want
-* something close to the Perceptual Quantizer (PQ) curve.
-*/
-   max = caps->aux_max_input_signal;
-   min = caps->aux_min_input_signal;
-
-   brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min
-  + user_brightness * max;
-   // Multiple the value by 1000 since we use millinits
-   brightness *= 1000;
-   brightness = DIV_ROUND_CLOSEST(brightness,
AMDGPU_MAX_BL_LEVEL);
+   // Firmware limits are 8-bit, PWM control is 16-bit.
+   *max = 0x101 * caps->max_input_signal;
+   *min = 0x101 * caps->min_input_signal;
}
+   return 1;
+}
   -out:
-   

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

On 2020-08-03 4:02 p.m., Alexander Monakov wrote:

Documentation for sysfs backlight level interface requires that
values in both 'brightness' and 'actual_brightness' files are
interpreted to be in range from 0 to the value given in the
'max_brightness' file.

With amdgpu, max_brightness gives 255, and values written by the user
into 'brightness' are internally rescaled to a wider range. However,
reading from 'actual_brightness' gives the raw register value without
inverse rescaling. This causes issues for various userspace tools such
as PowerTop and systemd that expect the value to be in the correct
range.

Introduce a helper to retrieve internal backlight range. Extend the
existing 'convert_brightness' function to handle conversion in both
directions.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242
Cc: Alex Deucher 
Signed-off-by: Alexander Monakov 


Overall approach seems reasonable, nice catch.

I suggest to add convert_to_user_brightness() instead of making 
from_user a flag and extending the current functionality though. It 
makes it more clear from the call site what's happening.


Regards,
Nicholas Kazlauskas


---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 73 ---
  1 file changed, 32 insertions(+), 41 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 710edc70e37e..03e21e7b7917 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2881,51 +2881,42 @@ static int set_backlight_via_aux(struct dc_link *link, 
uint32_t brightness)
return rc ? 0 : 1;
  }
  
-static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,

- const uint32_t user_brightness)
+static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
+   unsigned *min, unsigned *max)
  {
-   u32 min, max, conversion_pace;
-   u32 brightness = user_brightness;
-
if (!caps)
-   goto out;
+   return 0;
  
-	if (!caps->aux_support) {

-   max = caps->max_input_signal;
-   min = caps->min_input_signal;
-   /*
-* The brightness input is in the range 0-255
-* It needs to be rescaled to be between the
-* requested min and max input signal
-* It also needs to be scaled up by 0x101 to
-* match the DC interface which has a range of
-* 0 to 0x
-*/
-   conversion_pace = 0x101;
-   brightness =
-   user_brightness
-   * conversion_pace
-   * (max - min)
-   / AMDGPU_MAX_BL_LEVEL
-   + min * conversion_pace;
+   if (caps->aux_support) {
+   // Firmware limits are in nits, DC API wants millinits.
+   *max = 1000 * caps->aux_max_input_signal;
+   *min = 1000 * caps->aux_min_input_signal;
} else {
-   /* TODO
-* We are doing a linear interpolation here, which is OK but
-* does not provide the optimal result. We probably want
-* something close to the Perceptual Quantizer (PQ) curve.
-*/
-   max = caps->aux_max_input_signal;
-   min = caps->aux_min_input_signal;
-
-   brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min
-  + user_brightness * max;
-   // Multiple the value by 1000 since we use millinits
-   brightness *= 1000;
-   brightness = DIV_ROUND_CLOSEST(brightness, AMDGPU_MAX_BL_LEVEL);
+   // Firmware limits are 8-bit, PWM control is 16-bit.
+   *max = 0x101 * caps->max_input_signal;
+   *min = 0x101 * caps->min_input_signal;
}
+   return 1;
+}
  
-out:

-   return brightness;
+static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,
+ const uint32_t brightness, int from_user)
+{
+   unsigned min, max;
+
+   if (!get_brightness_range(caps, , ))
+   return brightness;
+
+   if (from_user)
+   // Rescale 0..255 to min..max
+   return min + DIV_ROUND_CLOSEST((max - min) * brightness,
+  AMDGPU_MAX_BL_LEVEL);
+
+   if (brightness < min)
+   return 0;
+   // Rescale min..max to 0..255
+   return DIV_ROUND_CLOSEST(AMDGPU_MAX_BL_LEVEL * (brightness - min),
+max - min);
  }
  
  static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)

@@ -2941,7 +2932,7 @@ static int amdgpu_dm_backlight_update_status(struct 
backlight_device *bd)

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

This is a cleaner change the other proposed patch since it doesn't need 
to modify the exist conversion functions but I'd be worried about broken 
userspace relying on 0-255 as the only acceptable range.


Not an expert on existing implementations to point out a specific one 
though.


Regards,
Nicholas Kazlauskas

On 2020-08-03 4:02 p.m., Alexander Monakov wrote:

Documentation for sysfs backlight level interface requires that
values in both 'brightness' and 'actual_brightness' files are
interpreted to be in range from 0 to the value given in the
'max_brightness' file.

With amdgpu, max_brightness gives 255, and values written by the user
into 'brightness' are internally rescaled to a wider range. However,
reading from 'actual_brightness' gives the raw register value without
inverse rescaling. This causes issues for various userspace tools such
as PowerTop and systemd that expect the value to be in the correct
range.

Introduce a helper to retrieve internal backlight range. Extend the
existing 'convert_brightness' function to handle conversion in both
directions.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242
Cc: Alex Deucher 
Signed-off-by: Alexander Monakov 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 73 ---
  1 file changed, 32 insertions(+), 41 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 710edc70e37e..03e21e7b7917 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2881,51 +2881,42 @@ static int set_backlight_via_aux(struct dc_link *link, 
uint32_t brightness)
return rc ? 0 : 1;
  }
  
-static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,

- const uint32_t user_brightness)
+static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
+   unsigned *min, unsigned *max)
  {
-   u32 min, max, conversion_pace;
-   u32 brightness = user_brightness;
-
if (!caps)
-   goto out;
+   return 0;
  
-	if (!caps->aux_support) {

-   max = caps->max_input_signal;
-   min = caps->min_input_signal;
-   /*
-* The brightness input is in the range 0-255
-* It needs to be rescaled to be between the
-* requested min and max input signal
-* It also needs to be scaled up by 0x101 to
-* match the DC interface which has a range of
-* 0 to 0x
-*/
-   conversion_pace = 0x101;
-   brightness =
-   user_brightness
-   * conversion_pace
-   * (max - min)
-   / AMDGPU_MAX_BL_LEVEL
-   + min * conversion_pace;
+   if (caps->aux_support) {
+   // Firmware limits are in nits, DC API wants millinits.
+   *max = 1000 * caps->aux_max_input_signal;
+   *min = 1000 * caps->aux_min_input_signal;
} else {
-   /* TODO
-* We are doing a linear interpolation here, which is OK but
-* does not provide the optimal result. We probably want
-* something close to the Perceptual Quantizer (PQ) curve.
-*/
-   max = caps->aux_max_input_signal;
-   min = caps->aux_min_input_signal;
-
-   brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min
-  + user_brightness * max;
-   // Multiple the value by 1000 since we use millinits
-   brightness *= 1000;
-   brightness = DIV_ROUND_CLOSEST(brightness, AMDGPU_MAX_BL_LEVEL);
+   // Firmware limits are 8-bit, PWM control is 16-bit.
+   *max = 0x101 * caps->max_input_signal;
+   *min = 0x101 * caps->min_input_signal;
}
+   return 1;
+}
  
-out:

-   return brightness;
+static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,
+ const uint32_t brightness, int from_user)
+{
+   unsigned min, max;
+
+   if (!get_brightness_range(caps, , ))
+   return brightness;
+
+   if (from_user)
+   // Rescale 0..255 to min..max
+   return min + DIV_ROUND_CLOSEST((max - min) * brightness,
+  AMDGPU_MAX_BL_LEVEL);
+
+   if (brightness < min)
+   return 0;
+   // Rescale min..max to 0..255
+   return DIV_ROUND_CLOSEST(AMDGPU_MAX_BL_LEVEL * (brightness - min),
+max - min);
  }
  
  static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)

@@ -2941,7 +2932,7 @@ static int 

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

[Kazlauskas, Nicholas]

On 2020-07-28 5:08 a.m., dan...@ffwll.ch wrote:

On Mon, Jul 27, 2020 at 10:49:48PM -0400, Kazlauskas, Nicholas wrote:

On 2020-07-27 5:32 p.m., Daniel Vetter wrote:

On Mon, Jul 27, 2020 at 11:11 PM Mazin Rezk  wrote:


On Monday, July 27, 2020 4:29 PM, Daniel Vetter  wrote:


On Mon, Jul 27, 2020 at 9:28 PM Christian König
 wrote:


Am 27.07.20 um 16:05 schrieb Kazlauskas, Nicholas:

On 2020-07-27 9:39 a.m., Christian König wrote:

Am 27.07.20 um 07:40 schrieb Mazin Rezk:

This patch fixes a race condition that causes a use-after-free during
amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking
commits
are requested and the second one finishes before the first.
Essentially,
this bug occurs when the following sequence of events happens:

1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
deferred to the workqueue.

2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
deferred to the workqueue.

3. Commit #2 starts before commit #1, dm_state #1 is used in the
commit_tail and commit #2 completes, freeing dm_state #1.

4. Commit #1 starts after commit #2 completes, uses the freed dm_state
1 and dereferences a freelist pointer while setting the context.


Well I only have a one mile high view on this, but why don't you let
the work items execute in order?

That would be better anyway cause this way we don't trigger a cache
line ping pong between CPUs.

Christian.


We use the DRM helpers for managing drm_atomic_commit_state and those
helpers internally push non-blocking commit work into the system
unbound work queue.


Mhm, well if you send those helper atomic commits in the order A,B and
they execute it in the order B,A I would call that a bug :)


The way it works is it pushes all commits into unbound work queue, but
then forces serialization as needed. We do _not_ want e.g. updates on
different CRTC to be serialized, that would result in lots of judder.
And hw is funny enough that there's all kinds of dependencies.

The way you force synchronization is by adding other CRTC state
objects. So if DC is busted and can only handle a single update per
work item, then I guess you always need all CRTC states and everything
will be run in order. But that also totally kills modern multi-screen
compositors. Xorg isn't modern, just in case that's not clear :-)

Lucking at the code it seems like you indeed have only a single dm
state, so yeah global sync is what you'll need as immediate fix, and
then maybe fix up DM to not be quite so silly ... or at least only do
the dm state stuff when really needed.

We could also sprinkle the drm_crtc_commit structure around a bit
(it's the glue that provides the synchronization across commits), but
since your dm state is global just grabbing all crtc states
unconditionally as part of that is probably best.


While we could duplicate a copy of that code with nothing but the
workqueue changed that isn't something I'd really like to maintain
going forward.


I'm not talking about duplicating the code, I'm talking about fixing the
helpers. I don't know that code well, but from the outside it sounds
like a bug there.

And executing work items in the order they are submitted is trivial.

Had anybody pinged Daniel or other people familiar with the helper code
about it?


Yeah something is wrong here, and the fix looks horrible :-)

Aside, I've also seen some recent discussion flare up about
drm_atomic_state_get/put used to paper over some other use-after-free,
but this time related to interrupt handlers. Maybe a few rules about
that:
- dont
- especially not when it's interrupt handlers, because you can't call
drm_atomic_state_put from interrupt handlers.

Instead have an spin_lock_irq to protect the shared date with your
interrupt handler, and _copy_ the date over. This is e.g. what
drm_crtc_arm_vblank_event does.


Nicholas wrote a patch that attempted to resolve the issue by adding every
CRTC into the commit to use use the stall checks. [1] While this forces
synchronisation on commits, it's kind of a hacky method that may take a
toll on performance.

Is it possible to have a DRM helper that forces synchronisation on some
commits without having to add every CRTC into the commit?

Also, is synchronisation really necessary for fast updates in amdgpu?
I'll admit, the idea of eliminating the use-after-free bug by eliminating
the use entirely doesn't seem ideal; but is forcing synchronisation on
these updates that much better?


Well clearing the dc_state pointer here and then allocating another
one in atomic_commit_tail also looks fishy. The proper fix is probably
a lot more involved, but yeah interim fix is to grab all crtc states
iff you also grabbed the dm_atomic_state structure. Real fix is to
only do this when necessary, which pretty much means the dc_state
needs to be somehow split up, or there needs to be some guarantees
about when it's necessary and when not. Otherwise parallel commits on
different CRTC are not possible with the current dc

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

[Kazlauskas, Nicholas]

On 2020-07-28 5:22 a.m., Paul Menzel wrote:

Dear Linux folks,


Am 25.07.20 um 07:20 schrieb Mazin Rezk:

On Saturday, July 25, 2020 12:59 AM, Duncan wrote:


On Sat, 25 Jul 2020 03:03:52 + Mazin Rezk wrote:


Am 24.07.20 um 19:33 schrieb Kees Cook:


There was a fix to disable the async path for this driver that
worked around the bug too, yes? That seems like a safer and more
focused change that doesn't revert the SLUB defense for all
users, and would actually provide a complete, I think, workaround


That said, I haven't seen the async disabling patch. If you could
link to it, I'd be glad to test it out and perhaps we can use that
instead.


I'm confused. Not to put words in Kees' mouth; /I/ am confused (which
admittedly could well be just because I make no claims to be a
coder and am simply reading the bug and thread, but I'd appreciate some
"unconfusing" anyway).

My interpretation of the "async disabling" reference was that it was to
comment #30 on the bug:

https://bugzilla.kernel.org/show_bug.cgi?id=207383#c30 



... which (if I'm not confused on this point too) appears to be yours.
There it was stated...

I've also found that this bug exclusively occurs when commit_work is on
the workqueue. After forcing drm_atomic_helper_commit to run all of the
commits without adding to the workqueue and running the OS, the issue
seems to have disappeared.


Would not forcing all commits to run directly, without placing them on
the workqueue, be "async disabling"? That's what I /thought/ he was
referencing.


Oh, I thought he was referring to a different patch. Kees, could I get
your confirmation on this?

The change I made actually affected all of the DRM code, although this 
could

easily be changed to be specific to amdgpu. (By forcing blocking on
amdgpu_dm's non-blocking commit code)

That said, I'd still need to test further because I only did test it 
for a

couple of hours then. Although it should work in theory.


OTOH your base/context swap idea sounds like a possibly "less
disturbance" workaround, if it works, and given the point in the
commit cycle... (But if it's out Sunday it's likely too late to test
and get it in now anyway; if it's another week, tho...)


The base/context swap idea should make the use-after-free behave how it
did in 5.6. Since the bug doesn't cause an issue in 5.6, it's less of a
"less disturbance" workaround and more of a "no disturbance" workaround.


Sorry for bothering, but is there now a solution, besides reverting the 
commits, to avoid freezes/crashes *without* performance regressions?



Kind regards,

Paul


Mazin's "drm/amd/display: Clear dm_state for fast updates" change 
accomplishes this, at least as a temporary hack.


I've started work on a more large scale fix that we could get in in after.

Regards,
Nicholas Kazlauskas

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

[Kazlauskas, Nicholas]

On 2020-07-27 5:32 p.m., Daniel Vetter wrote:

On Mon, Jul 27, 2020 at 11:11 PM Mazin Rezk  wrote:


On Monday, July 27, 2020 4:29 PM, Daniel Vetter  wrote:


On Mon, Jul 27, 2020 at 9:28 PM Christian König
 wrote:


Am 27.07.20 um 16:05 schrieb Kazlauskas, Nicholas:

On 2020-07-27 9:39 a.m., Christian König wrote:

Am 27.07.20 um 07:40 schrieb Mazin Rezk:

This patch fixes a race condition that causes a use-after-free during
amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking
commits
are requested and the second one finishes before the first.
Essentially,
this bug occurs when the following sequence of events happens:

1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
deferred to the workqueue.

2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
deferred to the workqueue.

3. Commit #2 starts before commit #1, dm_state #1 is used in the
commit_tail and commit #2 completes, freeing dm_state #1.

4. Commit #1 starts after commit #2 completes, uses the freed dm_state
1 and dereferences a freelist pointer while setting the context.


Well I only have a one mile high view on this, but why don't you let
the work items execute in order?

That would be better anyway cause this way we don't trigger a cache
line ping pong between CPUs.

Christian.


We use the DRM helpers for managing drm_atomic_commit_state and those
helpers internally push non-blocking commit work into the system
unbound work queue.


Mhm, well if you send those helper atomic commits in the order A,B and
they execute it in the order B,A I would call that a bug :)


The way it works is it pushes all commits into unbound work queue, but
then forces serialization as needed. We do _not_ want e.g. updates on
different CRTC to be serialized, that would result in lots of judder.
And hw is funny enough that there's all kinds of dependencies.

The way you force synchronization is by adding other CRTC state
objects. So if DC is busted and can only handle a single update per
work item, then I guess you always need all CRTC states and everything
will be run in order. But that also totally kills modern multi-screen
compositors. Xorg isn't modern, just in case that's not clear :-)

Lucking at the code it seems like you indeed have only a single dm
state, so yeah global sync is what you'll need as immediate fix, and
then maybe fix up DM to not be quite so silly ... or at least only do
the dm state stuff when really needed.

We could also sprinkle the drm_crtc_commit structure around a bit
(it's the glue that provides the synchronization across commits), but
since your dm state is global just grabbing all crtc states
unconditionally as part of that is probably best.


While we could duplicate a copy of that code with nothing but the
workqueue changed that isn't something I'd really like to maintain
going forward.


I'm not talking about duplicating the code, I'm talking about fixing the
helpers. I don't know that code well, but from the outside it sounds
like a bug there.

And executing work items in the order they are submitted is trivial.

Had anybody pinged Daniel or other people familiar with the helper code
about it?


Yeah something is wrong here, and the fix looks horrible :-)

Aside, I've also seen some recent discussion flare up about
drm_atomic_state_get/put used to paper over some other use-after-free,
but this time related to interrupt handlers. Maybe a few rules about
that:
- dont
- especially not when it's interrupt handlers, because you can't call
drm_atomic_state_put from interrupt handlers.

Instead have an spin_lock_irq to protect the shared date with your
interrupt handler, and _copy_ the date over. This is e.g. what
drm_crtc_arm_vblank_event does.


Nicholas wrote a patch that attempted to resolve the issue by adding every
CRTC into the commit to use use the stall checks. [1] While this forces
synchronisation on commits, it's kind of a hacky method that may take a
toll on performance.

Is it possible to have a DRM helper that forces synchronisation on some
commits without having to add every CRTC into the commit?

Also, is synchronisation really necessary for fast updates in amdgpu?
I'll admit, the idea of eliminating the use-after-free bug by eliminating
the use entirely doesn't seem ideal; but is forcing synchronisation on
these updates that much better?


Well clearing the dc_state pointer here and then allocating another
one in atomic_commit_tail also looks fishy. The proper fix is probably
a lot more involved, but yeah interim fix is to grab all crtc states
iff you also grabbed the dm_atomic_state structure. Real fix is to
only do this when necessary, which pretty much means the dc_state
needs to be somehow split up, or there needs to be some guarantees
about when it's necessary and when not. Otherwise parallel commits on
different CRTC are not possible with the current dc backend code.


Thanks for spending some time to help take a look at this as well.

The DRM documentation (at least

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

[Kazlauskas, Nicholas]

On 2020-07-27 9:39 a.m., Christian König wrote:

Am 27.07.20 um 07:40 schrieb Mazin Rezk:

This patch fixes a race condition that causes a use-after-free during
amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits
are requested and the second one finishes before the first. Essentially,
this bug occurs when the following sequence of events happens:

1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
deferred to the workqueue.

2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
deferred to the workqueue.

3. Commit #2 starts before commit #1, dm_state #1 is used in the
commit_tail and commit #2 completes, freeing dm_state #1.

4. Commit #1 starts after commit #2 completes, uses the freed dm_state
1 and dereferences a freelist pointer while setting the context.


Well I only have a one mile high view on this, but why don't you let the 
work items execute in order?


That would be better anyway cause this way we don't trigger a cache line 
ping pong between CPUs.


Christian.


We use the DRM helpers for managing drm_atomic_commit_state and those 
helpers internally push non-blocking commit work into the system unbound 
work queue.


While we could duplicate a copy of that code with nothing but the 
workqueue changed that isn't something I'd really like to maintain going 
forward.


Regards,
Nicholas Kazlauskas





Since this bug has only been spotted with fast commits, this patch fixes
the bug by clearing the dm_state instead of using the old dc_state for
fast updates. In addition, since dm_state is only used for its dc_state
and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is 
found,

removing the dm_state should not have any consequences in fast updates.

This use-after-free bug has existed for a while now, but only caused a
noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate
freelist pointer to middle of object") moving the freelist pointer from
dm_state->base (which was unused) to dm_state->context (which is
dereferenced).

Bugzilla: 
https://bugzilla.kernel.org/show_bug.cgi?id=207383 

Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for 
fast updates")

Reported-by: Duncan <1i5t5.dun...@cox.net>
Signed-off-by: Mazin Rezk 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++-
  1 file changed, 27 insertions(+), 9 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 86ffa0c2880f..710edc70e37e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct 
drm_device *dev,

   * the same resource. If we have a new DC context as part of
   * the DM atomic state from validation we need to free it and
   * retain the existing one instead.
+ *
+ * Furthermore, since the DM atomic state only contains the DC
+ * context and can safely be annulled, we can free the state
+ * and clear the associated private object now to free
+ * some memory and avoid a possible use-after-free later.
   */
-    struct dm_atomic_state *new_dm_state, *old_dm_state;

-    new_dm_state = dm_atomic_get_new_state(state);
-    old_dm_state = dm_atomic_get_old_state(state);
+    for (i = 0; i < state->num_private_objs; i++) {
+    struct drm_private_obj *obj = state->private_objs[i].ptr;

-    if (new_dm_state && old_dm_state) {
-    if (new_dm_state->context)
-    dc_release_state(new_dm_state->context);
+    if (obj->funcs == adev->dm.atomic_obj.funcs) {
+    int j = state->num_private_objs-1;

-    new_dm_state->context = old_dm_state->context;
+    dm_atomic_destroy_state(obj,
+    state->private_objs[i].state);
+
+    /* If i is not at the end of the array then the
+ * last element needs to be moved to where i was
+ * before the array can safely be truncated.
+ */
+    if (i != j)
+    state->private_objs[i] =
+    state->private_objs[j];

-    if (old_dm_state->context)
-    dc_retain_state(old_dm_state->context);
+    state->private_objs[j].ptr = NULL;
+    state->private_objs[j].state = NULL;
+    state->private_objs[j].old_state = NULL;
+    state->private_objs[j].new_state = NULL;
+
+    state->num_private_objs = j;
+    break;
+    }
  }
  }

--
2.27.0

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

[Kazlauskas, Nicholas]

On 2020-07-27 1:40 a.m., Mazin Rezk wrote:

This patch fixes a race condition that causes a use-after-free during
amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits
are requested and the second one finishes before the first. Essentially,
this bug occurs when the following sequence of events happens:

1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
deferred to the workqueue.

2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
deferred to the workqueue.

3. Commit #2 starts before commit #1, dm_state #1 is used in the
commit_tail and commit #2 completes, freeing dm_state #1.

4. Commit #1 starts after commit #2 completes, uses the freed dm_state
1 and dereferences a freelist pointer while setting the context.

Since this bug has only been spotted with fast commits, this patch fixes
the bug by clearing the dm_state instead of using the old dc_state for
fast updates. In addition, since dm_state is only used for its dc_state
and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is found,
removing the dm_state should not have any consequences in fast updates.

This use-after-free bug has existed for a while now, but only caused a
noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate
freelist pointer to middle of object") moving the freelist pointer from
dm_state->base (which was unused) to dm_state->context (which is
dereferenced).

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207383
Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for fast 
updates")
Reported-by: Duncan <1i5t5.dun...@cox.net>
Signed-off-by: Mazin Rezk 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++-
  1 file changed, 27 insertions(+), 9 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 86ffa0c2880f..710edc70e37e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct drm_device 
*dev,
 * the same resource. If we have a new DC context as part of
 * the DM atomic state from validation we need to free it and
 * retain the existing one instead.
+*
+* Furthermore, since the DM atomic state only contains the DC
+* context and can safely be annulled, we can free the state
+* and clear the associated private object now to free
+* some memory and avoid a possible use-after-free later.
 */
-   struct dm_atomic_state *new_dm_state, *old_dm_state;

-   new_dm_state = dm_atomic_get_new_state(state);
-   old_dm_state = dm_atomic_get_old_state(state);
+   for (i = 0; i < state->num_private_objs; i++) {
+   struct drm_private_obj *obj = 
state->private_objs[i].ptr;

-   if (new_dm_state && old_dm_state) {
-   if (new_dm_state->context)
-   dc_release_state(new_dm_state->context);
+   if (obj->funcs == adev->dm.atomic_obj.funcs) {
+   int j = state->num_private_objs-1;

-   new_dm_state->context = old_dm_state->context;
+   dm_atomic_destroy_state(obj,
+   state->private_objs[i].state);
+
+   /* If i is not at the end of the array then the
+* last element needs to be moved to where i was
+* before the array can safely be truncated.
+*/
+   if (i != j)
+   state->private_objs[i] =
+   state->private_objs[j];

-   if (old_dm_state->context)
-   dc_retain_state(old_dm_state->context);
+   state->private_objs[j].ptr = NULL;
+   state->private_objs[j].state = NULL;
+   state->private_objs[j].old_state = NULL;
+   state->private_objs[j].new_state = NULL;
+
+   state->num_private_objs = j;
+   break;
+   }


In the bug report itself I mentioned that I don't really like hacking 
around the DRM core for resolving this patch but to go into more 
specifics, it's really two issues of code maintenance:


1. It's iterating over internal structures and layout of private objects 
in the state and modifying the state. The core doesn't really guarantee 
how these things are going to be laid out and it may change in the future.


2. It's freeing an allocation we 

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

[Kazlauskas, Nicholas]

On 2020-07-23 5:10 p.m., Mazin Rezk wrote:

When amdgpu_dm_atomic_commit_tail is running in the workqueue,
drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
running, causing a race condition where state (and then dm_state) is
sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
occurred since 5.7-rc1 and is well documented among polaris11 users [1].

Prior to 5.7, this was not a noticeable issue since the freelist pointer
was stored at the beginning of dm_state (base), which was unused. After
changing the freelist pointer to be stored in the middle of the struct, the
freelist pointer overwrote the context, causing dc_state to become garbage
data and made the call to dm_enable_per_frame_crtc_master_sync dereference
a freelist pointer.

This patch fixes the aforementioned issue by calling drm_atomic_state_get
in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.

According to my testing on 5.8.0-rc6, this should fix bug 207383 on
Bugzilla [1].

[1] https://bugzilla.kernel.org/show_bug.cgi?id=207383

Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
Reported-by: Duncan <1i5t5.dun...@cox.net>
Signed-off-by: Mazin Rezk 


Thanks for the investigation and your patch. I appreciate the help in 
trying to narrow down the root cause as this issue has been difficult to 
reproduce on my setups.


Though I'm not sure this really resolves the issue - we make use of the 
drm_atomic_helper_commit helper function from DRM which internally does 
what you're doing with this patch:


drm_atomic_state_get(state);
if (nonblock)
queue_work(system_unbound_wq, >commit_work);
else
commit_tail(state);

So even when it gets queued off to the unbound workqueue we still have a 
reference on the state.


That reference gets dropped as part of commit tail helper in DRM as well:

if (funcs && funcs->atomic_commit_tail)
funcs->atomic_commit_tail(old_state);
else
drm_atomic_helper_commit_tail(old_state);

commit_time_ms = ktime_ms_delta(ktime_get(), start);
if (commit_time_ms > 0)
drm_self_refresh_helper_update_avg_times(old_state,
 (unsigned long)commit_time_ms,
 new_self_refresh_mask);

drm_atomic_helper_commit_cleanup_done(old_state);

drm_atomic_state_put(old_state);

So instead of a use after free happening when we access the state we get 
a double-free happening later at the end of commit tail in DRM.


What I think would be the right next step here is to actually determine 
what sequence of IOCTLs and atomic commits are happening under your 
setup with a very verbose dmesg log. You can set a debug level for DRM 
in your kernel parameters with something like:


drm.debug=0x54

I don't see anything in amdgpu_dm.c that looks like it would be freeing 
the state so I suspect something in the core is this doing this.



---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
  1 file changed, 3 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 86ffa0c2880f..86d6652872f2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7303,6 +7303,7 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
 * unset legacy_cursor_update
 */

+   drm_atomic_state_get(state);


Also note that if the drm_atomic_helper_commit() call fails here then 
we're going to never free this structure. So we should really be 
checking the return code here below before trying to do this, if at all.


Regards,
Nicholas Kazlauskas


return drm_atomic_helper_commit(dev, state, nonblock);

/*TODO Handle EINTR, reenable IRQ*/
@@ -7628,6 +7629,8 @@ static void amdgpu_dm_atomic_commit_tail(struct 
drm_atomic_state *state)

if (dc_state_temp)
dc_release_state(dc_state_temp);
+
+   drm_atomic_state_put(state);
  }


--
2.27.0

Re: [PATCH] drm/amd/display: add dmcub check on RENOIR

[Kazlauskas, Nicholas]

Looks good to me.

Reviewed-by: Nicholas Kazlauskas 

Regards,
Nicholas Kazlauskas

On 2020-07-08 10:15 a.m., Deucher, Alexander wrote:

[AMD Public Use]


[AMD Public Use]


Acked-by: Alex Deucher 

*From:* Aaron Ma 
*Sent:* Wednesday, July 8, 2020 4:16 AM
*To:* Wentland, Harry ; Li, Sun peng (Leo) 
; Deucher, Alexander ; 
Koenig, Christian ; airl...@linux.ie 
; dan...@ffwll.ch ; 
amd-...@lists.freedesktop.org ; 
dri-de...@lists.freedesktop.org ; 
linux-kernel@vger.kernel.org ; 
mapen...@gmail.com ; aaron...@canonical.com 


*Subject:* [PATCH] drm/amd/display: add dmcub check on RENOIR
RENOIR loads dmub fw not dmcu, check dmcu only will prevent loading iram,
it breaks backlight control.

Bug: 
https://bugzilla.kernel.org/show_bug.cgi?id=208277 


Signed-off-by: Aaron Ma 
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

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 10ac8076d4f2..db5e0bb0d935 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1358,7 +1358,7 @@ static int dm_late_init(void *handle)
  struct dmcu *dmcu = NULL;
  bool ret;

-   if (!adev->dm.fw_dmcu)
+   if (!adev->dm.fw_dmcu && !adev->dm.dmub_fw)
  return detect_mst_link_for_all_connectors(adev->ddev);

  dmcu = adev->dm.dc->res_pool->dmcu;
--
2.25.1


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 16/17] drm/amdgpu: gpu recovery does full modesets

[Kazlauskas, Nicholas]

On 2020-05-12 12:12 p.m., Daniel Vetter wrote:

On Tue, May 12, 2020 at 4:24 PM Alex Deucher  wrote:


On Tue, May 12, 2020 at 9:45 AM Daniel Vetter  wrote:


On Tue, May 12, 2020 at 3:29 PM Alex Deucher  wrote:


On Tue, May 12, 2020 at 9:17 AM Daniel Vetter  wrote:


On Tue, May 12, 2020 at 3:12 PM Alex Deucher  wrote:


On Tue, May 12, 2020 at 8:58 AM Daniel Vetter  wrote:


On Tue, May 12, 2020 at 08:54:45AM -0400, Alex Deucher wrote:

On Tue, May 12, 2020 at 5:00 AM Daniel Vetter  wrote:


...

I think it's time to stop this little exercise.

The lockdep splat, for the record:

[  132.583381] ==
[  132.584091] WARNING: possible circular locking dependency detected
[  132.584775] 5.7.0-rc3+ #346 Tainted: GW
[  132.585461] --
[  132.586184] kworker/2:3/865 is trying to acquire lock:
[  132.586857] c9677c70 (crtc_ww_class_acquire){+.+.}-{0:0}, at: 
drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper]
[  132.587569]
but task is already holding lock:
[  132.589044] 82318c80 (dma_fence_map){}-{0:0}, at: 
drm_sched_job_timedout+0x25/0xf0 [gpu_sched]
[  132.589803]
which lock already depends on the new lock.

[  132.592009]
the existing dependency chain (in reverse order) is:
[  132.593507]
-> #2 (dma_fence_map){}-{0:0}:
[  132.595019]dma_fence_begin_signalling+0x50/0x60
[  132.595767]drm_atomic_helper_commit+0xa1/0x180 [drm_kms_helper]
[  132.596567]drm_client_modeset_commit_atomic+0x1ea/0x250 [drm]
[  132.597420]drm_client_modeset_commit_locked+0x55/0x190 [drm]
[  132.598178]drm_client_modeset_commit+0x24/0x40 [drm]
[  132.598948]drm_fb_helper_restore_fbdev_mode_unlocked+0x4b/0xa0 
[drm_kms_helper]
[  132.599738]drm_fb_helper_set_par+0x30/0x40 [drm_kms_helper]
[  132.600539]fbcon_init+0x2e8/0x660
[  132.601344]visual_init+0xce/0x130
[  132.602156]do_bind_con_driver+0x1bc/0x2b0
[  132.602970]do_take_over_console+0x115/0x180
[  132.603763]do_fbcon_takeover+0x58/0xb0
[  132.604564]register_framebuffer+0x1ee/0x300
[  132.605369]__drm_fb_helper_initial_config_and_unlock+0x36e/0x520 
[drm_kms_helper]
[  132.606187]amdgpu_fbdev_init+0xb3/0xf0 [amdgpu]
[  132.607032]amdgpu_device_init.cold+0xe90/0x1677 [amdgpu]
[  132.607862]amdgpu_driver_load_kms+0x5a/0x200 [amdgpu]
[  132.608697]amdgpu_pci_probe+0xf7/0x180 [amdgpu]
[  132.609511]local_pci_probe+0x42/0x80
[  132.610324]pci_device_probe+0x104/0x1a0
[  132.611130]really_probe+0x147/0x3c0
[  132.611939]driver_probe_device+0xb6/0x100
[  132.612766]device_driver_attach+0x53/0x60
[  132.613593]__driver_attach+0x8c/0x150
[  132.614419]bus_for_each_dev+0x7b/0xc0
[  132.615249]bus_add_driver+0x14c/0x1f0
[  132.616071]driver_register+0x6c/0xc0
[  132.616902]do_one_initcall+0x5d/0x2f0
[  132.617731]do_init_module+0x5c/0x230
[  132.618560]load_module+0x2981/0x2bc0
[  132.619391]__do_sys_finit_module+0xaa/0x110
[  132.620228]do_syscall_64+0x5a/0x250
[  132.621064]entry_SYSCALL_64_after_hwframe+0x49/0xb3
[  132.621903]
-> #1 (crtc_ww_class_mutex){+.+.}-{3:3}:
[  132.623587]__ww_mutex_lock.constprop.0+0xcc/0x10c0
[  132.624448]ww_mutex_lock+0x43/0xb0
[  132.625315]drm_modeset_lock+0x44/0x120 [drm]
[  132.626184]drmm_mode_config_init+0x2db/0x8b0 [drm]
[  132.627098]amdgpu_device_init.cold+0xbd1/0x1677 [amdgpu]
[  132.628007]amdgpu_driver_load_kms+0x5a/0x200 [amdgpu]
[  132.628920]amdgpu_pci_probe+0xf7/0x180 [amdgpu]
[  132.629804]local_pci_probe+0x42/0x80
[  132.630690]pci_device_probe+0x104/0x1a0
[  132.631583]really_probe+0x147/0x3c0
[  132.632479]driver_probe_device+0xb6/0x100
[  132.633379]device_driver_attach+0x53/0x60
[  132.634275]__driver_attach+0x8c/0x150
[  132.635170]bus_for_each_dev+0x7b/0xc0
[  132.636069]bus_add_driver+0x14c/0x1f0
[  132.636974]driver_register+0x6c/0xc0
[  132.637870]do_one_initcall+0x5d/0x2f0
[  132.638765]do_init_module+0x5c/0x230
[  132.639654]load_module+0x2981/0x2bc0
[  132.640522]__do_sys_finit_module+0xaa/0x110
[  132.641372]do_syscall_64+0x5a/0x250
[  132.642203]entry_SYSCALL_64_after_hwframe+0x49/0xb3
[  132.643022]
-> #0 (crtc_ww_class_acquire){+.+.}-{0:0}:
[  132.644643]__lock_acquire+0x1241/0x23f0
[  132.645469]lock_acquire+0xad/0x370
[  132.646274]drm_modeset_acquire_init+0xd2/0x100 [drm]
[  132.647071]drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper]
[  132.647902]dm_suspend+0x1c/0x60 [amdgpu]
[  132.648698]

Re: [PATCH] [v2] amdgpu: fix gcc-4.8 build warnings

[Kazlauskas, Nicholas]

On 2020-04-29 5:20 a.m., Arnd Bergmann wrote:

Older compilers warn about initializers with incorrect curly
braces:

drivers/gpu/drm/drm_dp_mst_topology.c: In function 
'drm_dp_mst_dsc_aux_for_port':
drivers/gpu/drm/drm_dp_mst_topology.c:5497:9: error: missing braces around 
initializer [-Werror=missing-braces]
   struct drm_dp_desc desc = { 0 };
  ^

Change all instances in the amd gpu driver to using the GNU empty
initializer extension.


These should actually be memset - instead of GCC complaining, it'll be 
clang instead.


Regards,
Nicholas Kazlauskas



Reviewed-by: Rodrigo Siqueira 
Signed-off-by: Arnd Bergmann 
---
v2: some context changes linux-next stopped yesterday's patch from
applying today.
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
  drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c| 2 +-
  drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 2 +-
  drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c  | 6 +++---
  drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 6 +++---
  drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 2 +-
  drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hubp.c | 6 +++---
  7 files changed, 13 insertions(+), 13 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 7f4417981bff..81ce3103d751 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8695,7 +8695,7 @@ bool amdgpu_dm_psr_enable(struct dc_stream_state *stream)
  {
struct dc_link *link = stream->link;
unsigned int vsync_rate_hz = 0;
-   struct dc_static_screen_params params = {0};
+   struct dc_static_screen_params params = { };
/* Calculate number of static frames before generating interrupt to
 * enter PSR.
 */
diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c 
b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
index 37fa7b48250e..5484a316eaa8 100644
--- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
+++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
@@ -294,7 +294,7 @@ static enum bp_result bios_parser_get_i2c_info(struct 
dc_bios *dcb,
struct atom_display_object_path_v2 *object;
struct atom_common_record_header *header;
struct atom_i2c_record *record;
-   struct atom_i2c_record dummy_record = {0};
+   struct atom_i2c_record dummy_record = { };
struct bios_parser *bp = BP_FROM_DCB(dcb);
  
  	if (!info)

diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c 
b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
index 24c5765890fa..ee3ef5094fd1 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
@@ -698,7 +698,7 @@ void rn_clk_mgr_construct(
struct dccg *dccg)
  {
struct dc_debug_options *debug = >dc->debug;
-   struct dpm_clocks clock_table = { 0 };
+   struct dpm_clocks clock_table = { };
  
  	clk_mgr->base.ctx = ctx;

clk_mgr->base.funcs = _funcs;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index 9ef9e50a34fa..7cbfe740a947 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
@@ -2683,9 +2683,9 @@ static void dp_test_send_link_test_pattern(struct dc_link 
*link)
  
  static void dp_test_get_audio_test_data(struct dc_link *link, bool disable_video)

  {
-   union audio_test_modedpcd_test_mode = {0};
-   struct audio_test_pattern_type   dpcd_pattern_type = {0};
-   union audio_test_pattern_period  
dpcd_pattern_period[AUDIO_CHANNELS_COUNT] = {0};
+   union audio_test_modedpcd_test_mode = { };
+   struct audio_test_pattern_type   dpcd_pattern_type = { };
+   union audio_test_pattern_period  
dpcd_pattern_period[AUDIO_CHANNELS_COUNT] = { };
enum dp_test_pattern test_pattern = 
DP_TEST_PATTERN_AUDIO_OPERATOR_DEFINED;
  
  	struct pipe_ctx *pipes = link->dc->current_state->res_ctx.pipe_ctx;

diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c
index 84d7ac5dd206..dfa541f0b0d3 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c
@@ -1253,9 +1253,9 @@ void hubp2_validate_dml_output(struct hubp *hubp,
struct _vcs_dpi_display_ttu_regs_st *dml_ttu_attr)
  {
struct dcn20_hubp *hubp2 = TO_DCN20_HUBP(hubp);
-   struct _vcs_dpi_display_rq_regs_st rq_regs = {0};
-   struct _vcs_dpi_display_dlg_regs_st dlg_attr = {0};
-   struct _vcs_dpi_display_ttu_regs_st ttu_attr = {0};
+   struct _vcs_dpi_display_rq_regs_st rq_regs = { };
+   struct _vcs_dpi_display_dlg_regs_st dlg_attr = { };
+ 

Re: [PATCH 3/5] drm/amd: fix fb references in async update

[Kazlauskas, Nicholas]

On 3/4/19 9:49 AM, Helen Koike wrote:
> Async update callbacks are expected to set the old_fb in the new_state
> so prepare/cleanup framebuffers are balanced.
> 
> Calling drm_atomic_set_fb_for_plane() (which gets a reference of the new
> fb and put the old fb) is not required, as it's taken care by
> drm_mode_cursor_universal() when calling drm_atomic_helper_update_plane().
> 
> Suggested-by: Boris Brezillon 
> Signed-off-by: Helen Koike 

Reviewed-by: Nicholas Kazlauskas 

I guess the swap itself should be enough here as per the commit description.

It would have been nice if this patch dropped the old_plane_state->fb != 
new_plane_state->fb check too at the same time, but I suppose I can drop 
that later. It'll help us pass those failing IGT tests as well.

Nicholas Kazlauskas


> 
> ---
> Hello,
> 
> As mentioned in the cover letter,
> I tested on the rockchip and on i915 using igt plane_cursor_legacy and
> kms_cursor_legacy and I didn't see any regressions.
> But I couldn't test on AMD because I don't have the hardware and I would
> appreciate if anyone could test it.
> 
> Also, I didn't CC to stable here as I saw the async_update function was only
> added on v4.20, please let me know if I should CC to stable.
> 
> Thanks!
> Helen
> 
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
>   1 file changed, 1 insertion(+), 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 3a6f595f295e..bc02800254bf 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3760,8 +3760,7 @@ static void dm_plane_atomic_async_update(struct 
> drm_plane *plane,
>   struct drm_plane_state *old_state =
>   drm_atomic_get_old_plane_state(new_state->state, plane);
>   
> - if (plane->state->fb != new_state->fb)
> - drm_atomic_set_fb_for_plane(plane->state, new_state->fb);
> + swap(plane->state->fb, new_state->fb);
>   
>   plane->state->src_x = new_state->src_x;
>   plane->state->src_y = new_state->src_y;
> 

Re: [PATCH 1/5] drm: don't block fb changes for async plane updates

[Kazlauskas, Nicholas]

On 3/4/19 9:49 AM, Helen Koike wrote:
> In the case of a normal sync update, the preparation of framebuffers (be
> it calling drm_atomic_helper_prepare_planes() or doing setups with
> drm_framebuffer_get()) are performed in the new_state and the respective
> cleanups are performed in the old_state.
> 
> In the case of async updates, the preparation is also done in the
> new_state but the cleanups are done in the new_state (because updates
> are performed in place, i.e. in the current state).
> 
> The current code blocks async udpates when the fb is changed, turning
> async updates into sync updates, slowing down cursor updates and
> introducing regressions in igt tests with errors of type:
> 
> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
> expect to complete approximately 15360 updates, with the threshold set
> at 7680"
> 
> Fb changes in async updates were prevented to avoid the following scenario:
> 
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 (wrong)
> Where we have a single call to prepare fb2 but double cleanup call to fb2.
> 
> To solve the above problems, instead of blocking async fb changes, we
> place the old framebuffer in the new_state object, so when the code
> performs cleanups in the new_state it will cleanup the old_fb and we
> will have the following scenario instead:
> 
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, no cleanup
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb1
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
> 
> Where calls to prepare/cleanup are ballanced.
> 
> Cc:  # v4.14+: 25dc194b34dd: drm: Block fb changes 
> for async plane updates
> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
> Suggested-by: Boris Brezillon 
> Signed-off-by: Helen Koike 
> 
> ---
> Hello,
> 
> As mentioned in the cover letter,
> I tested on the rockchip and on i915 (with a patch I am still working on for
> replacing cursors by async update), with igt plane_cursor_legacy and
> kms_cursor_legacy and I didn't see any regressions.
> I couldn't test on MSM and AMD because I don't have the hardware (and I am
> having some issues testing on vc4) and I would appreciate if anyone could help
> me testing those.
> 
> I also think it would be a better solution if, instead of having async
> to do in-place updates in the current state, the async path should be
> equivalent to a syncronous update, i.e., modifying new_state and
> performing a flip
> IMHO, the only difference between sync and async should be that async update
> doesn't wait for vblank and applies the changes immeditally to the hw,
> but the code path could be almost the same.
> But for now I think this solution is ok (swaping new_fb/old_fb), and
> then we can adjust things little by little, what do you think?
> 
> Thanks!
> Helen
> 
>   drivers/gpu/drm/drm_atomic_helper.c | 20 ++--
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 540a77a2ade9..e7eb96f1efc2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1608,15 +1608,6 @@ int drm_atomic_helper_async_check(struct drm_device 
> *dev,
>   old_plane_state->crtc != new_plane_state->crtc)
>   return -EINVAL;
>   
> - /*
> -  * FIXME: Since prepare_fb and cleanup_fb are always called on
> -  * the new_plane_state for async updates we need to block framebuffer
> -  * changes. This prevents use of a fb that's been cleaned up and
> -  * double cleanups from occuring.
> -  */
> - if (old_plane_state->fb != new_plane_state->fb)
> - return -EINVAL;
> -
>   funcs = plane->helper_private;
>   if (!funcs->atomic_async_update)
>   return -EINVAL;
> @@ -1657,6 +1648,9 @@ void drm_atomic_helper_async_commit(struct drm_device 
> *dev,
>   int i;
>   
>   for_each_new_plane_in_state(state, plane, plane_state, i) {
> + struct drm_framebuffer *new_fb = plane_state->fb;
> + struct drm_framebuffer *old_fb = plane->state->fb;
> +
>   funcs = plane->helper_private;
>   funcs->atomic_async_update(plane, plane_state);
>   
> @@ -1665,11 +1659,17 @@ void drm_atomic_helper_async_commit(struct drm_device 
> *dev,
>* plane->state in-place, make sure at least common
>* properties have been properly updated.
>*/
> - WARN_ON_ONCE(plane->state->fb != plane_state->fb);
> + WARN_ON_ONCE(plane->state->fb != new_fb);
>   WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x);
>   WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y);
>   

Re: linux-next: build failure after merge of the drm-misc tree

[Kazlauskas, Nicholas]

On 1/8/19 3:37 AM, Daniel Vetter wrote:
> On Tue, Jan 08, 2019 at 11:12:41AM +1100, Stephen Rothwell wrote:
>> Hi all,
>>
>> After merging the drm-misc tree, today's linux-next build (x86_64
>> allmodconfig) failed like this:
>>
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
>> 'amdgpu_dm_mode_config_init':
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:1695:30: error: 
>> passing argument 1 of 'drm_atomic_private_obj_init' from incompatible 
>> pointer type [-Werror=incompatible-pointer-types]
>>drm_atomic_private_obj_init(>dm.atomic_obj,
>>^~~~
>> In file included from include/drm/drm_dp_mst_helper.h:27,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h:46,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu.h:57,
>>   from 
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:34:
>> include/drm/drm_atomic.h:437:53: note: expected 'struct drm_device *' but 
>> argument is of type 'struct drm_private_obj *'
>>   void drm_atomic_private_obj_init(struct drm_device *dev,
>>~~~^~~
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:1696:9: error: 
>> passing argument 2 of 'drm_atomic_private_obj_init' from incompatible 
>> pointer type [-Werror=incompatible-pointer-types]
>>   >base,
>>   ^~~~
>> In file included from include/drm/drm_dp_mst_helper.h:27,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h:46,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu.h:57,
>>   from 
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:34:
>> include/drm/drm_atomic.h:438:30: note: expected 'struct drm_private_obj *' 
>> but argument is of type 'struct drm_private_state *'
>>struct drm_private_obj *obj,
>>^~~
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:1697:9: error: 
>> passing argument 3 of 'drm_atomic_private_obj_init' from incompatible 
>> pointer type [-Werror=incompatible-pointer-types]
>>   _atomic_state_funcs);
>>   ^~
>> In file included from include/drm/drm_dp_mst_helper.h:27,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h:46,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu.h:57,
>>   from 
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:34:
>> include/drm/drm_atomic.h:439:32: note: expected 'struct drm_private_state *' 
>> but argument is of type 'struct drm_private_state_funcs *'
>>struct drm_private_state *state,
>>~~^
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:1695:2: error: 
>> too few arguments to function 'drm_atomic_private_obj_init'
>>drm_atomic_private_obj_init(>dm.atomic_obj,
>>^~~
>> In file included from include/drm/drm_dp_mst_helper.h:27,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h:46,
>>   from drivers/gpu/drm/amd/amdgpu/amdgpu.h:57,
>>   from 
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:34:
>> include/drm/drm_atomic.h:437:6: note: declared here
>>   void drm_atomic_private_obj_init(struct drm_device *dev,
>>^~~
>>
>> Caused by commit
>>
>>b962a12050a3 ("drm/atomic: integrate modeset lock with private objects")
>>
>> interacting with commit
>>
>>eb3dc8978596 ("drm/amd/display: Use private obj helpers for 
>> dm_atomic_state")
>>
>> from Linus' tree (merged during the merge window).
>>
>> Its not obvious how to fix this up, so I have used the drm-misc tree
>> from next-20190107 for today.
> 
> https://cgit.freedesktop.org/drm-tip/tree/fixups/drm-misc-next.patch?h=rerere-cache
> 
> is the fixup you want. Should get baked into drm-next any moment, since
> the first drm-misc-next pull is already out.
> -Daniel
>
Reviewed-by: Nicholas Kazlauskas 

Thanks, that lines up with the other driver fixes that patch added.

I missed that series when I was developing the private state object 
patch for amdgpu.

I'll still need to remove our extra lock since the objects now have 
their own, but that can come later in another non-fixup patch.

Nicholas Kazlauskas

Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE

[Kazlauskas, Nicholas]

On 12/20/18 12:09 PM, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher  wrote:
>>
>> + Harry
>>
>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter  wrote:
>>>
>>> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher  wrote:

 + Nicholas

 On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter  wrote:
>
> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa  wrote:
>>
>> Hi Helen,
>>
>> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike  
>> wrote:
>>>
>>> Hi Tomasz,
>>>
>>> On 12/13/18 2:02 AM, Tomasz Figa wrote:
 On Thu, Dec 6, 2018 at 1:12 AM Helen Koike  
 wrote:
>
> Hi Ville
>
> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
>>> Allow userspace to identify if the driver supports async update.
>>
>> And what exactly is an "async update"?
>
> I agree we are lacking docs on this, I'll send in the next version as
> soon as we agree on a name (please see below).
>
> For reference:
> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
>
>>
>> I keep asking people to come up with the a better name for this, and 
>> to
>> document what it actually means. Recently I've been think we should
>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
>> avoid introducing yet another set of names for the same thing. We'd
>> still want to document things properly though.
>
> Another name it was suggested was "atomic amend", this feature 
> basically
> allows userspace to complement an update previously sent (i.e. its in
> the queue and wasn't commited yet), it allows adding a plane update to
> the next commit. So what do you think in renaming it to "atomic 
> amend"?

 Note that it doesn't seem to be what the code currently is doing. For
 example, for cursor updates, it doesn't seem to be working on the
 currently pending commit, but just directly issues an atomic async
 update call to the planes. The code actually seems to fall back to a
 normal sync commit, if there is an already pending commit touching the
 same plane or including a modeset.
>>>
>>> It should fail as discussed at:
>>> https://patchwork.freedesktop.org/patch/243088/
>>>
>>> There was the following code inside the drm_atomic_helper_async_check()
>>> in the previous patch which would fallback to a normal commit if there
>>> isn't any pending commit to amend:
>>>
>>> +   if (!old_plane_state->commit)
>>> +   return -EINVAL;
>>>
>>> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
>>> this got removed, but which means that async update will be enabled
>>> anyway. So the following code is wrong:
>>>
>>> -   if (state->legacy_cursor_update)
>>> +   if (state->async_update || state->legacy_cursor_update)
>>>  state->async_update = 
>>> !drm_atomic_helper_async_check(dev, state);
>>>
>>> Does it make sense? If yes I'll fix this in the next version of the
>>> Atomic IOCTL patch (and also those two patches should be in the same
>>> series, I'll send them together next time).
>>>
>>> Thanks for pointing this out.
>>>
>>> Please let me know if you still don't agree on the name "atomic amend",
>>> or if I am missing something.
>>
>> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
>> need a way to commit the cursor plane asynchronously from other
>> commits any time the cursor changes its position or framebuffer. As
>> long as the new API allows that and the maintainers are fine with it,
>> I think I should be okay with it too.
>
> If this is just about the cursor, why is the current legacy cursor
> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> if we want to support having a normal atomic commit and a cursor
> update in the same atomic ioctl, coming up with reasonable semantics
> for that will be complicated.
>
> Pointer to code that uses this would be better ofc.

 I haven't followed this thread too closely, but we ended up needing to
 add a fast patch for cursor updates to amdgpu's atomic support to
 avoid stuttering issues.  Other drivers may end up being affected by
 this as well.  See:
 https://bugs.freedesktop.org/show_bug.cgi?id=106175
 Unfortunately, the fast path requires some hacks to handle the ref
 counting properly on cursor fbs:
 https://patchwork.freedesktop.org/patch/266138/
 https://patchwork.freedesktop.org/patch/268298/
 It looks like gamma may need similar treatment: