Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On Mon, Aug 10, 2020 at 4:13 PM Bas Nieuwenhuizen wrote: > > On Mon, Aug 10, 2020 at 3:09 PM Daniel Vetter wrote: > > > > On Mon, Aug 10, 2020 at 02:49:00PM +0200, Michel Dänzer wrote: > > > On 2020-08-10 2:28 p.m., Daniel Vetter wrote: > > > > > > > > Ok just learned that amdgpu hat set/get_tiling, so I'm upgrading my idea > > > > here to a very strong recommendation, i.e. please do this except if > > > > there's and amd ddx which somehow wants to change tiling mode while a fb > > > > exists, and expects this to propagate. > > > > > > > > In i915 we even disallow the set_tiling ioctl with an error if an fb > > > > exists, just to make sure userspace behaves. Even if userspace uses > > > > set_tiling, this way we can at least enforce the same semantics of > > > > "client > > > > can't pull compositor over the table with a set_tiling at the wrong > > > > time" > > > > of modifiers. > > > > > > FWIW, xf86-video-amdgpu doesn't have any code to set the tiling > > > metadata, only Mesa and presumably AMD's Vulkan/OpenGL UMDs do. > > > > Ah right you do everything with glamour, so this should never show up as a > > problem. > > I think it is a good idea to do so, but cannot do it completely in > this series as we don't define modifiers for GFX6-GFX8 GPU generations > yet. (wanted to leave these out for a bit to reduce the scope for the > initial version) Hm right, that makes it a bit awkward. > That said, there is a series that captures the tiling flags on FB > creation: https://patchwork.freedesktop.org/series/80109/ Yeah, but it only pushes it down into the state objects. Good first step, what I'm proposing is to push it all the way into addfb/struct drm_framebuffer. Since drm_framebuffer is also an invariant thing it makes the most sense to keep that there. I'm also discussing with Nicholas about what would be the ideal end state. But yeah maybe one thing at time. -Daniel > > > -Daniel > > -- > > 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 -- 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
Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On Mon, Aug 10, 2020 at 3:09 PM Daniel Vetter wrote: > > On Mon, Aug 10, 2020 at 02:49:00PM +0200, Michel Dänzer wrote: > > On 2020-08-10 2:28 p.m., Daniel Vetter wrote: > > > > > > Ok just learned that amdgpu hat set/get_tiling, so I'm upgrading my idea > > > here to a very strong recommendation, i.e. please do this except if > > > there's and amd ddx which somehow wants to change tiling mode while a fb > > > exists, and expects this to propagate. > > > > > > In i915 we even disallow the set_tiling ioctl with an error if an fb > > > exists, just to make sure userspace behaves. Even if userspace uses > > > set_tiling, this way we can at least enforce the same semantics of "client > > > can't pull compositor over the table with a set_tiling at the wrong time" > > > of modifiers. > > > > FWIW, xf86-video-amdgpu doesn't have any code to set the tiling > > metadata, only Mesa and presumably AMD's Vulkan/OpenGL UMDs do. > > Ah right you do everything with glamour, so this should never show up as a > problem. I think it is a good idea to do so, but cannot do it completely in this series as we don't define modifiers for GFX6-GFX8 GPU generations yet. (wanted to leave these out for a bit to reduce the scope for the initial version) That said, there is a series that captures the tiling flags on FB creation: https://patchwork.freedesktop.org/series/80109/ > -Daniel > -- > 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On Mon, Aug 10, 2020 at 02:49:00PM +0200, Michel Dänzer wrote: > On 2020-08-10 2:28 p.m., Daniel Vetter wrote: > > > > Ok just learned that amdgpu hat set/get_tiling, so I'm upgrading my idea > > here to a very strong recommendation, i.e. please do this except if > > there's and amd ddx which somehow wants to change tiling mode while a fb > > exists, and expects this to propagate. > > > > In i915 we even disallow the set_tiling ioctl with an error if an fb > > exists, just to make sure userspace behaves. Even if userspace uses > > set_tiling, this way we can at least enforce the same semantics of "client > > can't pull compositor over the table with a set_tiling at the wrong time" > > of modifiers. > > FWIW, xf86-video-amdgpu doesn't have any code to set the tiling > metadata, only Mesa and presumably AMD's Vulkan/OpenGL UMDs do. Ah right you do everything with glamour, so this should never show up as a problem. -Daniel -- 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
Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On 2020-08-10 2:28 p.m., Daniel Vetter wrote: > > Ok just learned that amdgpu hat set/get_tiling, so I'm upgrading my idea > here to a very strong recommendation, i.e. please do this except if > there's and amd ddx which somehow wants to change tiling mode while a fb > exists, and expects this to propagate. > > In i915 we even disallow the set_tiling ioctl with an error if an fb > exists, just to make sure userspace behaves. Even if userspace uses > set_tiling, this way we can at least enforce the same semantics of "client > can't pull compositor over the table with a set_tiling at the wrong time" > of modifiers. FWIW, xf86-video-amdgpu doesn't have any code to set the tiling metadata, only Mesa and presumably AMD's Vulkan/OpenGL UMDs do. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On Wed, Aug 05, 2020 at 09:32:10AM +0200, dan...@ffwll.ch wrote:
> On Tue, Aug 04, 2020 at 11:31:17PM +0200, Bas Nieuwenhuizen wrote:
> > This sets the DC tiling options from the modifier, if modifiers
> > are used for the FB. This patch by itself does not expose the
> > support yet though.
> >
> > There is not much validation yet to limit the scope of this
> > patch, but the current validation is at the same level as
> > the BO metadata path.
> >
> > Signed-off-by: Bas Nieuwenhuizen
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 109 +-
> > 1 file changed, 103 insertions(+), 6 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 6ef7f2f8acab..ac913b8f10ef 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3754,6 +3754,93 @@ fill_gfx9_plane_attributes_from_flags(struct
> > amdgpu_device *adev,
> > return 0;
> > }
> >
> > +static bool
> > +modifier_has_dcc(uint64_t modifier)
> > +{
> > + return IS_AMD_FMT_MOD(modifier) && AMD_FMT_MOD_GET(DCC, modifier);
> > +}
> > +
> > +static unsigned
> > +modifier_gfx9_swizzle_mode(uint64_t modifier)
> > +{
> > + if (modifier == DRM_FORMAT_MOD_LINEAR)
> > + return 0;
> > +
> > + return AMD_FMT_MOD_GET(TILE, modifier);
> > +}
> > +
> > +static void
> > +fill_gfx9_tiling_info_from_modifier(const struct amdgpu_device *adev,
> > + union dc_tiling_info *tiling_info,
> > + uint64_t modifier)
> > +{
> > + unsigned int mod_bank_xor_bits = AMD_FMT_MOD_GET(BANK_XOR_BITS,
> > modifier);
> > + unsigned int mod_pipe_xor_bits = AMD_FMT_MOD_GET(PIPE_XOR_BITS,
> > modifier);
> > + unsigned int pkrs_log2 = AMD_FMT_MOD_GET(PACKERS, modifier);
> > + unsigned int pipes_log2 = min(4u, mod_pipe_xor_bits);
> > +
> > + fill_gfx9_tiling_info_from_device(adev, tiling_info);
> > +
> > + if (!IS_AMD_FMT_MOD(modifier))
> > + return;
> > +
> > + tiling_info->gfx9.num_pipes = 1u << pipes_log2;
> > + tiling_info->gfx9.num_shader_engines = 1u << (mod_pipe_xor_bits -
> > pipes_log2);
> > +
> > + if (adev->family >= AMDGPU_FAMILY_NV) {
> > + tiling_info->gfx9.num_pkrs = 1u << pkrs_log2;
> > + } else {
> > + tiling_info->gfx9.num_banks = 1u << mod_bank_xor_bits;
> > +
> > + /* for DCC we know it isn't rb aligned, so rb_per_se doesn't
> > matter. */
> > + }
> > +}
> > +
> > +static void
> > +block_alignment(unsigned int blocksize_log2, unsigned int *width, unsigned
> > int *height)
> > +{
> > + unsigned int height_log2 = blocksize_log2 / 2;
> > + unsigned int width_log2 = blocksize_log2 - height_log2;
> > +
> > + *width = 1u << width_log2;
> > + *height = 1u << height_log2;
> > +}
> > +
> > +static int
> > +fill_gfx9_plane_attributes_from_modifiers(struct amdgpu_device *adev,
> > + const struct amdgpu_framebuffer *afb,
> > + const enum surface_pixel_format format,
> > + const enum dc_rotation_angle rotation,
> > + const struct plane_size *plane_size,
> > + union dc_tiling_info *tiling_info,
> > + struct dc_plane_dcc_param *dcc,
> > + struct dc_plane_address *address,
> > + const bool force_disable_dcc)
> > +{
> > + const uint64_t modifier = afb->base.modifier;
> > + int ret;
> > +
> > + fill_gfx9_tiling_info_from_modifier(adev, tiling_info, modifier);
> > + tiling_info->gfx9.swizzle = modifier_gfx9_swizzle_mode(modifier);
> > +
> > + if (modifier_has_dcc(modifier) && !force_disable_dcc) {
> > + uint64_t dcc_address = afb->address + afb->base.offsets[1];
> > +
> > + dcc->enable = 1;
> > + dcc->meta_pitch = afb->base.pitches[1];
> > + dcc->independent_64b_blks =
> > AMD_FMT_MOD_GET(DCC_INDEPENDENT_64B, modifier);
> > +
> > + address->grph.meta_addr.low_part = lower_32_bits(dcc_address);
> > + address->grph.meta_addr.high_part = upper_32_bits(dcc_address);
> > + }
> > +
> > + ret = validate_dcc(adev, format, rotation, tiling_info, dcc, address,
> > plane_size);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > fill_plane_buffer_attributes(struct amdgpu_device *adev,
> > const struct amdgpu_framebuffer *afb,
> > @@ -3823,12 +3910,22 @@ fill_plane_buffer_attributes(struct amdgpu_device
> > *adev,
> >
> >
> > if (adev->family >= AMDGPU_FAMILY_AI) {
> > - ret = fill_gfx9_plane_attributes_from_flags(adev, afb, format,
> > rotation,
> > - plane_size,
> > tiling_info, dcc,
> >
Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers.
On Tue, Aug 04, 2020 at 11:31:17PM +0200, Bas Nieuwenhuizen wrote:
> This sets the DC tiling options from the modifier, if modifiers
> are used for the FB. This patch by itself does not expose the
> support yet though.
>
> There is not much validation yet to limit the scope of this
> patch, but the current validation is at the same level as
> the BO metadata path.
>
> Signed-off-by: Bas Nieuwenhuizen
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 109 +-
> 1 file changed, 103 insertions(+), 6 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 6ef7f2f8acab..ac913b8f10ef 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3754,6 +3754,93 @@ fill_gfx9_plane_attributes_from_flags(struct
> amdgpu_device *adev,
> return 0;
> }
>
> +static bool
> +modifier_has_dcc(uint64_t modifier)
> +{
> + return IS_AMD_FMT_MOD(modifier) && AMD_FMT_MOD_GET(DCC, modifier);
> +}
> +
> +static unsigned
> +modifier_gfx9_swizzle_mode(uint64_t modifier)
> +{
> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> + return 0;
> +
> + return AMD_FMT_MOD_GET(TILE, modifier);
> +}
> +
> +static void
> +fill_gfx9_tiling_info_from_modifier(const struct amdgpu_device *adev,
> + union dc_tiling_info *tiling_info,
> + uint64_t modifier)
> +{
> + unsigned int mod_bank_xor_bits = AMD_FMT_MOD_GET(BANK_XOR_BITS,
> modifier);
> + unsigned int mod_pipe_xor_bits = AMD_FMT_MOD_GET(PIPE_XOR_BITS,
> modifier);
> + unsigned int pkrs_log2 = AMD_FMT_MOD_GET(PACKERS, modifier);
> + unsigned int pipes_log2 = min(4u, mod_pipe_xor_bits);
> +
> + fill_gfx9_tiling_info_from_device(adev, tiling_info);
> +
> + if (!IS_AMD_FMT_MOD(modifier))
> + return;
> +
> + tiling_info->gfx9.num_pipes = 1u << pipes_log2;
> + tiling_info->gfx9.num_shader_engines = 1u << (mod_pipe_xor_bits -
> pipes_log2);
> +
> + if (adev->family >= AMDGPU_FAMILY_NV) {
> + tiling_info->gfx9.num_pkrs = 1u << pkrs_log2;
> + } else {
> + tiling_info->gfx9.num_banks = 1u << mod_bank_xor_bits;
> +
> + /* for DCC we know it isn't rb aligned, so rb_per_se doesn't
> matter. */
> + }
> +}
> +
> +static void
> +block_alignment(unsigned int blocksize_log2, unsigned int *width, unsigned
> int *height)
> +{
> + unsigned int height_log2 = blocksize_log2 / 2;
> + unsigned int width_log2 = blocksize_log2 - height_log2;
> +
> + *width = 1u << width_log2;
> + *height = 1u << height_log2;
> +}
> +
> +static int
> +fill_gfx9_plane_attributes_from_modifiers(struct amdgpu_device *adev,
> + const struct amdgpu_framebuffer *afb,
> + const enum surface_pixel_format format,
> + const enum dc_rotation_angle rotation,
> + const struct plane_size *plane_size,
> + union dc_tiling_info *tiling_info,
> + struct dc_plane_dcc_param *dcc,
> + struct dc_plane_address *address,
> + const bool force_disable_dcc)
> +{
> + const uint64_t modifier = afb->base.modifier;
> + int ret;
> +
> + fill_gfx9_tiling_info_from_modifier(adev, tiling_info, modifier);
> + tiling_info->gfx9.swizzle = modifier_gfx9_swizzle_mode(modifier);
> +
> + if (modifier_has_dcc(modifier) && !force_disable_dcc) {
> + uint64_t dcc_address = afb->address + afb->base.offsets[1];
> +
> + dcc->enable = 1;
> + dcc->meta_pitch = afb->base.pitches[1];
> + dcc->independent_64b_blks =
> AMD_FMT_MOD_GET(DCC_INDEPENDENT_64B, modifier);
> +
> + address->grph.meta_addr.low_part = lower_32_bits(dcc_address);
> + address->grph.meta_addr.high_part = upper_32_bits(dcc_address);
> + }
> +
> + ret = validate_dcc(adev, format, rotation, tiling_info, dcc, address,
> plane_size);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> static int
> fill_plane_buffer_attributes(struct amdgpu_device *adev,
>const struct amdgpu_framebuffer *afb,
> @@ -3823,12 +3910,22 @@ fill_plane_buffer_attributes(struct amdgpu_device
> *adev,
>
>
> if (adev->family >= AMDGPU_FAMILY_AI) {
> - ret = fill_gfx9_plane_attributes_from_flags(adev, afb, format,
> rotation,
> - plane_size,
> tiling_info, dcc,
> - address,
> tiling_flags,
> - force_disable_dcc);
> - if (ret)
> -
[PATCH 6/8] drm/amd/display: Set DC options from modifiers.
This sets the DC tiling options from the modifier, if modifiers
are used for the FB. This patch by itself does not expose the
support yet though.
There is not much validation yet to limit the scope of this
patch, but the current validation is at the same level as
the BO metadata path.
Signed-off-by: Bas Nieuwenhuizen
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 109 +-
1 file changed, 103 insertions(+), 6 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 6ef7f2f8acab..ac913b8f10ef 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3754,6 +3754,93 @@ fill_gfx9_plane_attributes_from_flags(struct
amdgpu_device *adev,
return 0;
}
+static bool
+modifier_has_dcc(uint64_t modifier)
+{
+ return IS_AMD_FMT_MOD(modifier) && AMD_FMT_MOD_GET(DCC, modifier);
+}
+
+static unsigned
+modifier_gfx9_swizzle_mode(uint64_t modifier)
+{
+ if (modifier == DRM_FORMAT_MOD_LINEAR)
+ return 0;
+
+ return AMD_FMT_MOD_GET(TILE, modifier);
+}
+
+static void
+fill_gfx9_tiling_info_from_modifier(const struct amdgpu_device *adev,
+ union dc_tiling_info *tiling_info,
+ uint64_t modifier)
+{
+ unsigned int mod_bank_xor_bits = AMD_FMT_MOD_GET(BANK_XOR_BITS,
modifier);
+ unsigned int mod_pipe_xor_bits = AMD_FMT_MOD_GET(PIPE_XOR_BITS,
modifier);
+ unsigned int pkrs_log2 = AMD_FMT_MOD_GET(PACKERS, modifier);
+ unsigned int pipes_log2 = min(4u, mod_pipe_xor_bits);
+
+ fill_gfx9_tiling_info_from_device(adev, tiling_info);
+
+ if (!IS_AMD_FMT_MOD(modifier))
+ return;
+
+ tiling_info->gfx9.num_pipes = 1u << pipes_log2;
+ tiling_info->gfx9.num_shader_engines = 1u << (mod_pipe_xor_bits -
pipes_log2);
+
+ if (adev->family >= AMDGPU_FAMILY_NV) {
+ tiling_info->gfx9.num_pkrs = 1u << pkrs_log2;
+ } else {
+ tiling_info->gfx9.num_banks = 1u << mod_bank_xor_bits;
+
+ /* for DCC we know it isn't rb aligned, so rb_per_se doesn't
matter. */
+ }
+}
+
+static void
+block_alignment(unsigned int blocksize_log2, unsigned int *width, unsigned int
*height)
+{
+ unsigned int height_log2 = blocksize_log2 / 2;
+ unsigned int width_log2 = blocksize_log2 - height_log2;
+
+ *width = 1u << width_log2;
+ *height = 1u << height_log2;
+}
+
+static int
+fill_gfx9_plane_attributes_from_modifiers(struct amdgpu_device *adev,
+ const struct amdgpu_framebuffer *afb,
+ const enum surface_pixel_format format,
+ const enum dc_rotation_angle rotation,
+ const struct plane_size *plane_size,
+ union dc_tiling_info *tiling_info,
+ struct dc_plane_dcc_param *dcc,
+ struct dc_plane_address *address,
+ const bool force_disable_dcc)
+{
+ const uint64_t modifier = afb->base.modifier;
+ int ret;
+
+ fill_gfx9_tiling_info_from_modifier(adev, tiling_info, modifier);
+ tiling_info->gfx9.swizzle = modifier_gfx9_swizzle_mode(modifier);
+
+ if (modifier_has_dcc(modifier) && !force_disable_dcc) {
+ uint64_t dcc_address = afb->address + afb->base.offsets[1];
+
+ dcc->enable = 1;
+ dcc->meta_pitch = afb->base.pitches[1];
+ dcc->independent_64b_blks =
AMD_FMT_MOD_GET(DCC_INDEPENDENT_64B, modifier);
+
+ address->grph.meta_addr.low_part = lower_32_bits(dcc_address);
+ address->grph.meta_addr.high_part = upper_32_bits(dcc_address);
+ }
+
+ ret = validate_dcc(adev, format, rotation, tiling_info, dcc, address,
plane_size);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
static int
fill_plane_buffer_attributes(struct amdgpu_device *adev,
const struct amdgpu_framebuffer *afb,
@@ -3823,12 +3910,22 @@ fill_plane_buffer_attributes(struct amdgpu_device *adev,
if (adev->family >= AMDGPU_FAMILY_AI) {
- ret = fill_gfx9_plane_attributes_from_flags(adev, afb, format,
rotation,
- plane_size,
tiling_info, dcc,
- address,
tiling_flags,
- force_disable_dcc);
- if (ret)
- return ret;
+ if (afb->base.flags & DRM_MODE_FB_MODIFIERS) {
+ ret = fill_gfx9_plane_attributes_from_modifiers(adev,
afb, format,
+
