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