* [bug report] drm/amd/display: wait for fence without holding reservation lock
@ 2019-05-02 8:04 Dan Carpenter
2019-05-02 12:56 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2019-05-02 8:04 UTC (permalink / raw)
To: ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] drm/amd/display: wait for fence without holding reservation lock
2019-05-02 8:04 [bug report] drm/amd/display: wait for fence without holding reservation lock Dan Carpenter
@ 2019-05-02 12:56 ` Kazlauskas, Nicholas
0 siblings, 0 replies; 2+ messages in thread
From: Kazlauskas, Nicholas @ 2019-05-02 12:56 UTC (permalink / raw)
To: Dan Carpenter,
ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-05-02 12:56 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-02 8:04 [bug report] drm/amd/display: wait for fence without holding reservation lock Dan Carpenter
2019-05-02 12:56 ` Kazlauskas, Nicholas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.