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

[Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

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?

  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx