Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
On Mon, Aug 17, 2020 at 02:23:47AM -0400, Marek Olšák wrote: > On Wed, Aug 12, 2020 at 9:54 AM Daniel Vetter wrote: > > > 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? > > > > I thought we were only talking about shared buffers. > > If the other side is only a consumer and the producer must disable DCC, the > producer decompresses DCC and then disables it and updates the BO flags. > The consumer doesn't need the new flags, because even if DCC stays enabled > in the consumer, it's in a decompressed state (it has no effect). Only the > producer knows it's disabled, and any new consumer will also know it when > it queries the latest BO flags. > > It doesn't work if both sides use writes, because it's not communicated > that DCC is disabled (BO flags are queried only once). This hasn't been a > problem so far. > > Is there a way to disable DCC correctly and safely across processes? Yes. > So why don't we do it? Because it would add more GPU overhead. Yeah but in this case you can get away with just sampling the bo flags once (which is what you're doing), so doing that at addfb time should be perfectly fine. Ofc you might waste a bit of $something also scanning out the compression metadata (which tells the hw that it's all uncompressed), but that doesn't seem to be a problem for you. So treating the legacy bo flags as invariant for shared buffers should be perfectly fine. -Daniel -- 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
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
On Wed, Aug 12, 2020 at 9:54 AM Daniel Vetter wrote: > 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? > I thought we were only talking about shared buffers. If the other side is only a consumer and the producer must disable DCC, the producer decompresses DCC and then disables it and updates the BO flags. The consumer doesn't need the new flags, because even if DCC stays enabled in the consumer, it's in a decompressed state (it has no effect). Only the producer knows it's disabled, and any new consumer will also know it when it queries the latest BO flags. It doesn't work if both sides use writes, because it's not communicated that DCC is disabled (BO flags are queried only once). This hasn't been a problem so far. Is there a way to disable DCC correctly and safely across processes? Yes. So why don't we do it? Because it would add more GPU overhead. Marek ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
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.leichtzumer...@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., dan...@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
> > Cc: Rodrigo Siqueira
> > Signed-off-by: Nicholas Kazlauskas
> > ---
> > .../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, _flags);
> > -
> > - tmz_surface = amdgpu_bo_encrypted(rbo);
> > -
> > ttm_eu_backoff_reservation(, );
> > afb->address = amdgpu_bo_gpu_offset(rbo);
> > amdgpu_bo_ref(rbo);
> > + /**
> > + * We don't do surface updates on planes that have been newly
>
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
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.
Marek
On Mon., Aug. 10, 2020, 08:30 Christian König, <
ckoenig.leichtzumer...@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., dan...@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
> Cc: Rodrigo Siqueira
> Signed-off-by: Nicholas Kazlauskas
> ---
> .../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, _flags);
> -
> - tmz_surface = amdgpu_bo_encrypted(rbo);
> -
> ttm_eu_backoff_reservation(, );
> 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
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
On Mon, Aug 10, 2020 at 02:30:05PM +0200, Christian König 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., dan...@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.
Yeah it's the only reasonable model really, since set/get_tiling is not
synchronized with any winsys protocol. So full of races by design.
The only thing I'd worry about is if you do set_tiling after addfb in your
ddx. That one is save, but would badly break if we sample modifiers from
set_tiling flags only once at addfb time.
-Daniel
>
> 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
> > > > > Cc: Rodrigo Siqueira
> > > > > Signed-off-by: Nicholas Kazlauskas
> > > > > ---
> > > > >.../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, _flags);
> > > > > -
> > > > > - tmz_surface = amdgpu_bo_encrypted(rbo);
> > > > > -
> > > > > ttm_eu_backoff_reservation(, );
> > > > > 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
>
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
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., dan...@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
Cc: Rodrigo Siqueira
Signed-off-by: Nicholas Kazlauskas
---
.../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, _flags);
-
- tmz_surface = amdgpu_bo_encrypted(rbo);
-
ttm_eu_backoff_reservation(, );
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, _state->tiling_info,
- _state->plane_size, _state->dcc,
- _state->address, tmz_surface,
- force_disable_dcc);
+
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
On Fri, Aug 07, 2020 at 10:29:09AM -0400, Kazlauskas, Nicholas wrote:
> On 2020-08-07 4:30 a.m., dan...@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.
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
> > > Cc: Rodrigo Siqueira
> > > Signed-off-by: Nicholas Kazlauskas
> > > ---
> > > .../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, _flags);
> > > -
> > > - tmz_surface = amdgpu_bo_encrypted(rbo);
> > > -
> > > ttm_eu_backoff_reservation(, );
> > > 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,
> > > -
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
On 2020-08-07 4:30 a.m., dan...@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.
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
Cc: Rodrigo Siqueira
Signed-off-by: Nicholas Kazlauskas
---
.../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, _flags);
-
- tmz_surface = amdgpu_bo_encrypted(rbo);
-
ttm_eu_backoff_reservation(, );
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, _state->tiling_info,
- _state->plane_size, _state->dcc,
- _state->address, tmz_surface,
- force_disable_dcc);
+ dm_plane_state_new->tiling_flags,
+ _state->tiling_info, _state->plane_size,
+ _state->dcc, _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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
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
>
> [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
> Cc: Rodrigo Siqueira
> Signed-off-by: Nicholas Kazlauskas
> ---
> .../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, _flags);
> -
> - tmz_surface = amdgpu_bo_encrypted(rbo);
> -
> ttm_eu_backoff_reservation(, );
>
> 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, _state->tiling_info,
> - _state->plane_size, _state->dcc,
> - _state->address, tmz_surface,
> - force_disable_dcc);
> + dm_plane_state_new->tiling_flags,
> + _state->tiling_info, _state->plane_size,
> + _state->dcc, _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
--
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
Re: [PATCH 3/7] drm/amd/display: Avoid using unvalidated tiling_flags and tmz_surface in prepare_planes
Reviewed-by: Rodrigo Siqueira
On 07/30, 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.
>
> [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
> Cc: Rodrigo Siqueira
> Signed-off-by: Nicholas Kazlauskas
> ---
> .../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, _flags);
> -
> - tmz_surface = amdgpu_bo_encrypted(rbo);
> -
> ttm_eu_backoff_reservation(, );
>
> 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, _state->tiling_info,
> - _state->plane_size, _state->dcc,
> - _state->address, tmz_surface,
> - force_disable_dcc);
> + dm_plane_state_new->tiling_flags,
> + _state->tiling_info, _state->plane_size,
> + _state->dcc, _state->address,
> + dm_plane_state_new->tmz_surface, force_disable_dcc);
> }
>
> return 0;
> --
> 2.25.1
>
--
Rodrigo Siqueira
https://siqueira.tech
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
