From: daniel@ffwll.ch Cc: amd-gfx@lists.freedesktop.org, maraeo@gmail.com, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers. Date: Wed, 5 Aug 2020 09:32:10 +0200 [thread overview] Message-ID: <20200805073210.GU6419@phenom.ffwll.local> (raw) In-Reply-To: <20200804213119.25091-7-bas@basnieuwenhuizen.nl> 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 <bas@basnieuwenhuizen.nl> > --- > .../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, > + rotation, plane_size, > + tiling_info, dcc, > + address, > + force_disable_dcc); > + if (ret) > + return ret; > + } else { > + 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; So what we've done in i915, but might be too cumbersome with the amdgpu modifiers, is to map legacy tiling information into modifiers once, at the beginning of addfb. So in amdgpu_display_user_framebuffer_create(). And once modifiers are filled in, you ofc set the DRM_MODE_FB_MODIFIERS flag too in the fb. Then all the display code only works with modifiers, instead of having a mix and possible confusion, with breakage when people only test the legacy path. Which I kinda expect to happen, since amd probably runs with their own ddx in their CI rig only. This also avoids a bunch of layering and locking unprettiness, since display code doesn't need to dig around in gem_bo side of things. On that, there's another amdgpu_bo_get_tiling_flags in amdgpu_dm_commit_planes which probably shouldn't be there, and should use computed stuff from plane state or fb (I changed it to a lockless version to just hack around locking issues, but still there). This hopefully/eventually should allow us to entirely phase out the legacy magic tiling blob attached to bo (we've pulled the trigger on that for intel now, would have needed to extend the uapi to keep it working was a good excuse). Cheers, Daniel > + } > } else { > fill_gfx8_tiling_info_from_flags(tiling_info, tiling_flags); > } > -- > 2.28.0 > -- 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@ffwll.ch Cc: amd-gfx@lists.freedesktop.org, maraeo@gmail.com, daniel@fooishbar.org, dri-devel@lists.freedesktop.org, daniel@ffwll.ch, harry.wentland@amd.com Subject: Re: [PATCH 6/8] drm/amd/display: Set DC options from modifiers. Date: Wed, 5 Aug 2020 09:32:10 +0200 [thread overview] Message-ID: <20200805073210.GU6419@phenom.ffwll.local> (raw) In-Reply-To: <20200804213119.25091-7-bas@basnieuwenhuizen.nl> 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 <bas@basnieuwenhuizen.nl> > --- > .../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, > + rotation, plane_size, > + tiling_info, dcc, > + address, > + force_disable_dcc); > + if (ret) > + return ret; > + } else { > + 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; So what we've done in i915, but might be too cumbersome with the amdgpu modifiers, is to map legacy tiling information into modifiers once, at the beginning of addfb. So in amdgpu_display_user_framebuffer_create(). And once modifiers are filled in, you ofc set the DRM_MODE_FB_MODIFIERS flag too in the fb. Then all the display code only works with modifiers, instead of having a mix and possible confusion, with breakage when people only test the legacy path. Which I kinda expect to happen, since amd probably runs with their own ddx in their CI rig only. This also avoids a bunch of layering and locking unprettiness, since display code doesn't need to dig around in gem_bo side of things. On that, there's another amdgpu_bo_get_tiling_flags in amdgpu_dm_commit_planes which probably shouldn't be there, and should use computed stuff from plane state or fb (I changed it to a lockless version to just hack around locking issues, but still there). This hopefully/eventually should allow us to entirely phase out the legacy magic tiling blob attached to bo (we've pulled the trigger on that for intel now, would have needed to extend the uapi to keep it working was a good excuse). Cheers, Daniel > + } > } else { > fill_gfx8_tiling_info_from_flags(tiling_info, tiling_flags); > } > -- > 2.28.0 > -- 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-05 7:32 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-04 21:31 [PATCH 0/8] amd/display: Add GFX9+ modifier support Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 1/8] drm/amd/display: Do not silently accept DCC for multiplane formats Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 2/8] drm/amd: Init modifier field of helper fb Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 3/8] drm/amd/display: Honor the offset for plane 0 Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-05 7:21 ` daniel 2020-08-05 7:21 ` daniel 2020-08-04 21:31 ` [PATCH 4/8] drm/fourcc: Add AMD DRM modifiers Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 5/8] drm/amd/display: Refactor surface tiling setup Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 6/8] drm/amd/display: Set DC options from modifiers Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-05 7:32 ` daniel [this message] 2020-08-05 7:32 ` daniel 2020-08-10 12:28 ` Daniel Vetter 2020-08-10 12:28 ` Daniel Vetter 2020-08-10 12:49 ` Michel Dänzer 2020-08-10 12:49 ` Michel Dänzer 2020-08-10 13:09 ` Daniel Vetter 2020-08-10 13:09 ` Daniel Vetter 2020-08-10 14:13 ` Bas Nieuwenhuizen 2020-08-10 14:13 ` Bas Nieuwenhuizen 2020-08-10 14:20 ` Daniel Vetter 2020-08-10 14:20 ` Daniel Vetter 2020-08-04 21:31 ` [PATCH 7/8] drm/amd/display: Add formats for DCC with 2/3 planes Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-04 21:31 ` [PATCH 8/8] drm/amd/display: Expose modifiers Bas Nieuwenhuizen 2020-08-04 21:31 ` Bas Nieuwenhuizen 2020-08-07 19:42 ` Marek Olšák 2020-08-07 19:42 ` Marek Olšák 2020-09-02 10:31 ` Bas Nieuwenhuizen 2020-09-02 10:31 ` Bas Nieuwenhuizen 2020-09-03 2:42 ` Marek Olšák 2020-09-03 2:42 ` Marek Olšák
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=20200805073210.GU6419@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=maraeo@gmail.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.