From: Daniel Vetter <daniel@ffwll.ch> To: "Marek Olšák" <maraeo@gmail.com> Cc: "Olsak, Marek" <Marek.Olsak@amd.com>, "Rodrigo Siqueira" <Rodrigo.Siqueira@amd.com>, amd-gfx@lists.freedesktop.org, "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>, dri-devel@lists.freedesktop.org, "Bhawanpreet Lakha" <Bhawanpreet.Lakha@amd.com>, "Christian König" <christian.koenig@amd.com> Subject: Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes Date: Wed, 12 Aug 2020 15:54:47 +0200 [thread overview] Message-ID: <20200812135447.GF2352366@phenom.ffwll.local> (raw) In-Reply-To: <CAAxE2A5BXVJ9xQ=C6F=Df1MCGUKTjV7yR=6x5hu6vfARp1SD7Q@mail.gmail.com> On Tue, Aug 11, 2020 at 09:42:11AM -0400, Marek Olšák wrote: > 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. But if that happens, how do you tell the other side that it needs to sample new flags? Does that just happen all the time? Also do the DDC state changes happen for shared buffers too? -Daniel > > Marek > > On Mon., Aug. 10, 2020, 08:30 Christian König, < > ckoenig.leichtzumerken@gmail.com> wrote: > > > 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., daniel@ffwll.ch 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 <Bhawanpreet.Lakha@amd.com> > > >>>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > > >>>> --- > > >>>> .../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 > > >>>> dri-devel@lists.freedesktop.org > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: "Marek Olšák" <maraeo@gmail.com> Cc: "Olsak, Marek" <Marek.Olsak@amd.com>, "Rodrigo Siqueira" <Rodrigo.Siqueira@amd.com>, amd-gfx@lists.freedesktop.org, "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>, dri-devel@lists.freedesktop.org, "Daniel Vetter" <daniel@ffwll.ch>, "Bhawanpreet Lakha" <Bhawanpreet.Lakha@amd.com>, "Christian König" <christian.koenig@amd.com> Subject: Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes Date: Wed, 12 Aug 2020 15:54:47 +0200 [thread overview] Message-ID: <20200812135447.GF2352366@phenom.ffwll.local> (raw) In-Reply-To: <CAAxE2A5BXVJ9xQ=C6F=Df1MCGUKTjV7yR=6x5hu6vfARp1SD7Q@mail.gmail.com> On Tue, Aug 11, 2020 at 09:42:11AM -0400, Marek Olšák wrote: > 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. But if that happens, how do you tell the other side that it needs to sample new flags? Does that just happen all the time? Also do the DDC state changes happen for shared buffers too? -Daniel > > Marek > > On Mon., Aug. 10, 2020, 08:30 Christian König, < > ckoenig.leichtzumerken@gmail.com> wrote: > > > 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., daniel@ffwll.ch 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 <Bhawanpreet.Lakha@amd.com> > > >>>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > > >>>> --- > > >>>> .../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 > > >>>> dri-devel@lists.freedesktop.org > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2020-08-12 13:54 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-30 20:36 [PATCH 0/7] drm/amd/display: Drop DRM private objects from amdgpu_dm Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-07-30 20:36 ` [PATCH 1/7] drm/amd/display: Store tiling_flags and tmz_surface on dm_plane_state Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-08-05 21:04 ` Rodrigo Siqueira 2020-08-05 21:04 ` Rodrigo Siqueira 2020-08-07 8:24 ` daniel 2020-08-07 8:24 ` daniel 2020-07-30 20:36 ` [PATCH 2/7] drm/amd/display: Reset plane when tiling flags change Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-07-30 20:50 ` Wu, Hersen 2020-07-30 20:50 ` Wu, Hersen 2020-08-05 21:11 ` Rodrigo Siqueira 2020-08-05 21:11 ` Rodrigo Siqueira 2020-08-06 18:18 ` Kazlauskas, Nicholas 2020-08-06 18:18 ` Kazlauskas, Nicholas 2020-07-30 20:36 ` [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-08-05 21:12 ` Rodrigo Siqueira 2020-08-05 21:12 ` Rodrigo Siqueira 2020-08-07 8:30 ` daniel 2020-08-07 8:30 ` daniel 2020-08-07 14:29 ` Kazlauskas, Nicholas 2020-08-07 14:29 ` Kazlauskas, Nicholas 2020-08-10 12:25 ` Daniel Vetter 2020-08-10 12:25 ` Daniel Vetter 2020-08-10 12:30 ` Christian König 2020-08-10 12:30 ` Christian König 2020-08-10 12:32 ` Daniel Vetter 2020-08-10 12:32 ` Daniel Vetter 2020-08-11 13:42 ` Marek Olšák 2020-08-11 13:42 ` Marek Olšák 2020-08-12 13:54 ` Daniel Vetter [this message] 2020-08-12 13:54 ` Daniel Vetter 2020-08-17 6:23 ` Marek Olšák 2020-08-17 6:23 ` Marek Olšák 2020-09-01 7:20 ` Daniel Vetter 2020-09-01 7:20 ` Daniel Vetter 2020-07-30 20:36 ` [PATCH 4/7] drm/amd/display: Use validated tiling_flags and tmz_surface in commit_tail Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-08-05 21:15 ` Rodrigo Siqueira 2020-08-05 21:15 ` Rodrigo Siqueira 2020-07-30 20:36 ` [PATCH 5/7] drm/amd/display: Reset plane for anything that's not a FAST update Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-07-30 20:51 ` Wu, Hersen 2020-07-30 20:51 ` Wu, Hersen 2020-08-05 20:45 ` Rodrigo Siqueira 2020-08-05 20:45 ` Rodrigo Siqueira 2020-08-06 18:27 ` Kazlauskas, Nicholas 2020-08-06 18:27 ` Kazlauskas, Nicholas 2020-08-07 8:34 ` daniel 2020-08-07 8:34 ` daniel 2020-08-07 14:26 ` Kazlauskas, Nicholas 2020-08-07 14:26 ` Kazlauskas, Nicholas 2020-08-10 12:30 ` Daniel Vetter 2020-08-10 12:30 ` Daniel Vetter 2020-07-30 20:36 ` [PATCH 6/7] drm/amd/display: Drop dm_determine_update_type_for_commit Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-08-05 20:48 ` Rodrigo Siqueira 2020-08-05 20:48 ` Rodrigo Siqueira 2020-07-30 20:36 ` [PATCH 7/7] drm/amd/display: Replace DRM private objects with subclassed DRM atomic state Nicholas Kazlauskas 2020-07-30 20:36 ` Nicholas Kazlauskas 2020-08-05 20:37 ` Rodrigo Siqueira 2020-08-05 20:37 ` Rodrigo Siqueira 2020-08-06 14:25 ` Kazlauskas, Nicholas 2020-08-06 14:25 ` Kazlauskas, Nicholas 2020-08-07 8:40 ` daniel 2020-08-07 8:40 ` daniel 2020-08-07 8:52 ` daniel 2020-08-07 8:52 ` daniel 2020-08-07 14:32 ` Kazlauskas, Nicholas 2020-08-07 14:32 ` Kazlauskas, Nicholas 2020-08-10 12:34 ` Daniel Vetter 2020-08-10 12:34 ` Daniel Vetter 2020-08-07 8:29 ` [PATCH 0/7] drm/amd/display: Drop DRM private objects from amdgpu_dm daniel 2020-08-07 8:29 ` daniel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200812135447.GF2352366@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=Bhawanpreet.Lakha@amd.com \ --cc=Marek.Olsak@amd.com \ --cc=Rodrigo.Siqueira@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=maraeo@gmail.com \ --cc=nicholas.kazlauskas@amd.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.