<div dir="auto">There are a few cases when the flags can change, for example DCC can be disabled due to a hw limitation in the 3d engine. Modifiers give the misleading impression that they help with that, but they don't. They don't really help with anything.<div dir="auto"> </div><div dir="auto">Marek</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon., Aug. 10, 2020, 08:30 Christian König, <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Am 10.08.20 um 14:25 schrieb Daniel Vetter: > On Fri, Aug 07, 2020 at 10:29:09AM -0400, Kazlauskas, Nicholas wrote: >> On 2020-08-07 4:30 a.m., <a href="mailto:daniel@ffwll.ch" target="_blank" rel="noreferrer">daniel@ffwll.ch</a> wrote: >>> On Thu, Jul 30, 2020 at 04:36:38PM -0400, Nicholas Kazlauskas wrote: >>>> [Why] >>>> We're racing with userspace as the flags could potentially change >>>> from when we acquired and validated them in commit_check. >>> Uh ... I didn't know these could change. I think my comments on Bas' >>> series are even more relevant now. I think long term would be best to bake >>> these flags in at addfb time when modifiers aren't set. And otherwise >>> always use the modifiers flag, and completely ignore the legacy flags >>> here. >>> -Daniel >>> >> There's a set tiling/mod flags IOCTL that can be called after addfb happens, >> so unless there's some sort of driver magic preventing this from working >> when it's already been pinned for scanout then I don't see anything stopping >> this from happening. >> >> I still need to review the modifiers series in a little more detail but that >> looks like a good approach to fixing these kind of issues. > Yeah we had the same model for i915, but it's awkward and can surprise > compositors (since the client could change the tiling mode from underneath > the compositor). So freezing the tiling mode at addfb time is the right > thing to do, and anyway how things work with modifiers. > > Ofc maybe good to audit the -amd driver, but hopefully it doesn't do > anything silly with changed tiling. If it does, it's kinda sad day. Marek should know this right away, but I think we only set the tilling flags once while exporting the BO and then never change them. Regards, Christian. > > Btw for i915 we even went a step further, and made the set_tiling ioctl > return an error if a framebuffer for that gem_bo existed. Just to make > sure we don't ever accidentally break this. > > Cheers, Daniel > >> Regards, >> Nicholas Kazlauskas >> >>>> [How] >>>> We unfortunately can't drop this function in its entirety from >>>> prepare_planes since we don't know the afb->address at commit_check >>>> time yet. >>>> >>>> So instead of querying new tiling_flags and tmz_surface use the ones >>>> from the plane_state directly. >>>> >>>> While we're at it, also update the force_disable_dcc option based >>>> on the state from atomic check. >>>> >>>> Cc: Bhawanpreet Lakha <<a href="mailto:Bhawanpreet.Lakha@amd.com" target="_blank" rel="noreferrer">Bhawanpreet.Lakha@amd.com</a>> >>>> Cc: Rodrigo Siqueira <<a href="mailto:Rodrigo.Siqueira@amd.com" target="_blank" rel="noreferrer">Rodrigo.Siqueira@amd.com</a>> >>>> Signed-off-by: Nicholas Kazlauskas <<a href="mailto:nicholas.kazlauskas@amd.com" target="_blank" rel="noreferrer">nicholas.kazlauskas@amd.com</a>> >>>> --- >>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++++++++++--------- >>>> 1 file changed, 19 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> index bf1881bd492c..f78c09c9585e 100644 >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> @@ -5794,14 +5794,8 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, >>>> struct list_head list; >>>> struct ttm_validate_buffer tv; >>>> struct ww_acquire_ctx ticket; >>>> - uint64_t tiling_flags; >>>> uint32_t domain; >>>> int r; >>>> - bool tmz_surface = false; >>>> - bool force_disable_dcc = false; >>>> - >>>> - dm_plane_state_old = to_dm_plane_state(plane->state); >>>> - dm_plane_state_new = to_dm_plane_state(new_state); >>>> if (!new_state->fb) { >>>> DRM_DEBUG_DRIVER("No FB bound\n"); >>>> @@ -5845,27 +5839,35 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, >>>> return r; >>>> } >>>> - amdgpu_bo_get_tiling_flags(rbo, &tiling_flags); >>>> - >>>> - tmz_surface = amdgpu_bo_encrypted(rbo); >>>> - >>>> ttm_eu_backoff_reservation(&ticket, &list); >>>> afb->address = amdgpu_bo_gpu_offset(rbo); >>>> amdgpu_bo_ref(rbo); >>>> + /** >>>> + * We don't do surface updates on planes that have been newly created, >>>> + * but we also don't have the afb->address during atomic check. >>>> + * >>>> + * Fill in buffer attributes depending on the address here, but only on >>>> + * newly created planes since they're not being used by DC yet and this >>>> + * won't modify global state. >>>> + */ >>>> + dm_plane_state_old = to_dm_plane_state(plane->state); >>>> + dm_plane_state_new = to_dm_plane_state(new_state); >>>> + >>>> if (dm_plane_state_new->dc_state && >>>> - dm_plane_state_old->dc_state != dm_plane_state_new->dc_state) { >>>> - struct dc_plane_state *plane_state = dm_plane_state_new->dc_state; >>>> + dm_plane_state_old->dc_state != dm_plane_state_new->dc_state) { >>>> + struct dc_plane_state *plane_state = >>>> + dm_plane_state_new->dc_state; >>>> + bool force_disable_dcc = !plane_state->dcc.enable; >>>> - force_disable_dcc = adev->asic_type == CHIP_RAVEN && adev->in_suspend; >>>> fill_plane_buffer_attributes( >>>> adev, afb, plane_state->format, plane_state->rotation, >>>> - tiling_flags, &plane_state->tiling_info, >>>> - &plane_state->plane_size, &plane_state->dcc, >>>> - &plane_state->address, tmz_surface, >>>> - force_disable_dcc); >>>> + dm_plane_state_new->tiling_flags, >>>> + &plane_state->tiling_info, &plane_state->plane_size, >>>> + &plane_state->dcc, &plane_state->address, >>>> + dm_plane_state_new->tmz_surface, force_disable_dcc); >>>> } >>>> return 0; >>>> -- >>>> 2.25.1 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank" rel="noreferrer">dri-devel@lists.freedesktop.org</a> >>>> <a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> _______________________________________________ amd-gfx mailing list <a href="mailto:amd-gfx@lists.freedesktop.org" target="_blank" rel="noreferrer">amd-gfx@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" rel="noreferrer noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </blockquote></div>