[bug report] drm/amd/display: wait for fence without holding reservation lock
Dan Carpenter
dan.carpenter at oracle.com
Thu May 2 08:04:42 UTC 2019
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
More information about the amd-gfx
mailing list