[bug report] drm/amd/display: wait for fence without holding reservation lock

Kazlauskas, Nicholas Nicholas.Kazlauskas at amd.com
Thu May 2 12:56:27 UTC 2019


On 5/2/19 4:04 AM, Dan Carpenter wrote:
> [CAUTION: External Email]
> 
> Hello Christian K├Ânig,
> 
> The patch 2fac0f53fe59: "drm/amd/display: wait for fence without
> holding reservation lock" from Apr 2, 2019, leads to the following
> static checker warning:
> 
>          drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5338 amdgpu_dm_commit_planes()
>          warn: 'r' unsigned <= 0
> 
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
>    5238  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>    5239                                      struct dc_state *dc_state,
>    5240                                      struct drm_device *dev,
>    5241                                      struct amdgpu_display_manager *dm,
>    5242                                      struct drm_crtc *pcrtc,
>    5243                                      bool wait_for_vblank)
>    5244  {
>    5245          uint32_t i, r;
>                  ^^^^^^^^    ^
> 
>    5246          uint64_t timestamp_ns;
>    5247          struct drm_plane *plane;
>    5248          struct drm_plane_state *old_plane_state, *new_plane_state;
>    5249          struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
>    5250          struct drm_crtc_state *new_pcrtc_state =
>    5251                          drm_atomic_get_new_crtc_state(state, pcrtc);
>    5252          struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>    5253          struct dm_crtc_state *dm_old_crtc_state =
>    5254                          to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
>    5255          int planes_count = 0, vpos, hpos;
>    5256          unsigned long flags;
>    5257          struct amdgpu_bo *abo;
>    5258          uint64_t tiling_flags;
>    5259          uint32_t target_vblank, last_flip_vblank;
>    5260          bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
>    5261          bool pflip_present = false;
>    5262          struct {
>    5263                  struct dc_surface_update surface_updates[MAX_SURFACES];
>    5264                  struct dc_plane_info plane_infos[MAX_SURFACES];
>    5265                  struct dc_scaling_info scaling_infos[MAX_SURFACES];
>    5266                  struct dc_flip_addrs flip_addrs[MAX_SURFACES];
>    5267                  struct dc_stream_update stream_update;
>    5268          } *bundle;
>    5269
>    5270          bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
>    5271
>    5272          if (!bundle) {
>    5273                  dm_error("Failed to allocate update bundle\n");
>    5274                  goto cleanup;
>    5275          }
>    5276
>    5277          /*
>    5278           * Disable the cursor first if we're disabling all the planes.
>    5279           * It'll remain on the screen after the planes are re-enabled
>    5280           * if we don't.
>    5281           */
>    5282          if (acrtc_state->active_planes == 0)
>    5283                  amdgpu_dm_commit_cursors(state);
>    5284
>    5285          /* update planes when needed */
>    5286          for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>    5287                  struct drm_crtc *crtc = new_plane_state->crtc;
>    5288                  struct drm_crtc_state *new_crtc_state;
>    5289                  struct drm_framebuffer *fb = new_plane_state->fb;
>    5290                  bool plane_needs_flip;
>    5291                  struct dc_plane_state *dc_plane;
>    5292                  struct dm_plane_state *dm_new_plane_state = to_dm_plane_state(new_plane_state);
>    5293
>    5294                  /* Cursor plane is handled after stream updates */
>    5295                  if (plane->type == DRM_PLANE_TYPE_CURSOR)
>    5296                          continue;
>    5297
>    5298                  if (!fb || !crtc || pcrtc != crtc)
>    5299                          continue;
>    5300
>    5301                  new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>    5302                  if (!new_crtc_state->active)
>    5303                          continue;
>    5304
>    5305                  dc_plane = dm_new_plane_state->dc_state;
>    5306
>    5307                  bundle->surface_updates[planes_count].surface = dc_plane;
>    5308                  if (new_pcrtc_state->color_mgmt_changed) {
>    5309                          bundle->surface_updates[planes_count].gamma = dc_plane->gamma_correction;
>    5310                          bundle->surface_updates[planes_count].in_transfer_func = dc_plane->in_transfer_func;
>    5311                  }
>    5312
>    5313                  fill_dc_scaling_info(new_plane_state,
>    5314                                       &bundle->scaling_infos[planes_count]);
>    5315
>    5316                  bundle->surface_updates[planes_count].scaling_info =
>    5317                          &bundle->scaling_infos[planes_count];
>    5318
>    5319                  plane_needs_flip = old_plane_state->fb && new_plane_state->fb;
>    5320
>    5321                  pflip_present = pflip_present || plane_needs_flip;
>    5322
>    5323                  if (!plane_needs_flip) {
>    5324                          planes_count += 1;
>    5325                          continue;
>    5326                  }
>    5327
>    5328                  abo = gem_to_amdgpu_bo(fb->obj[0]);
>    5329
>    5330                  /*
>    5331                   * Wait for all fences on this FB. Do limited wait to avoid
>    5332                   * deadlock during GPU reset when this fence will not signal
>    5333                   * but we hold reservation lock for the BO.
>    5334                   */
>    5335                  r = reservation_object_wait_timeout_rcu(abo->tbo.resv, true,
>    5336                                                          false,
>    5337                                                          msecs_to_jiffies(5000));
>    5338                  if (unlikely(r <= 0))
>                              ^^^^^^^^^^^^^^^^
> 
> If reservation_object_wait_timeout_rcu() then r is unsigned.  Also this
> just prints an error instead of doing proper error handling so it's not
> really worth fixing?

Looks like that isn't even the only place in the function with this 
problem. There's that r != 0 check right below as well casted from an int.

I'll push a patch that fixes both of these up.

Nicholas Kazlauskas

> 
>    5339                          DRM_ERROR("Waiting for fences timed out or interrupted!");
>    5340
>    5341                  /*
>    5342                   * TODO This might fail and hence better not used, wait
>    5343                   * explicitly on fences instead
>    5344                   * and in general should be called for
>    5345                   * blocking commit to as per framework helpers
>    5346                   */
>    5347                  r = amdgpu_bo_reserve(abo, true);
>    5348                  if (unlikely(r != 0))
>    5349                          DRM_ERROR("failed to reserve buffer before flip\n");
>    5350
>    5351                  amdgpu_bo_get_tiling_flags(abo, &tiling_flags);
>    5352
>    5353                  amdgpu_bo_unreserve(abo);
> 
> regards,
> dan carpenter
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 



More information about the amd-gfx mailing list