On Fri, Nov 13, 2020 at 03:56:41PM -0500, Bindu Ramamurthy wrote:
> From: Wayne Lin <wayne....@amd.com>
>
> [Why]
> Instead of calculating CRC on whole frame, add flexibility to calculate
> CRC on specific frame region.
>
> [How]
> Add few crc window coordinate properties. By default, CRC is calculated
> on whole frame unless user space specifies the CRC calculation window.
>
> Signed-off-by: Wayne Lin <wayne....@amd.com>
> Acked-by: Bindu Ramamurthy <bind...@amd.com>
Already pinged Alex on irc, but here also as a mail.
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 142 +++++++++++++++++-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 19 +++
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 43 +++++-
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h | 3 +
> drivers/gpu/drm/amd/display/dc/core/dc_link.c | 3 +
> 5 files changed, 201 insertions(+), 9 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 77c06f999040..f81c49f28bc9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -943,6 +943,41 @@ static void mmhub_read_system_context(struct
> amdgpu_device *adev, struct dc_phy_
> }
> #endif
>
> +#ifdef CONFIG_DEBUG_FS
> +static int create_crtc_crc_properties(struct amdgpu_display_manager *dm)
Yes it's behind a #ifdef but a) most distros enable this anyway and b)
it's still a KMS property, so still uapi, i.e.
- should be discussed on dri-devel
- needs igt testcases and stuff
- and real userspace
Drivers adding random kms properties has brought us into a pretty giant
mess, we need to stop this. That's why we've increased merge criteria for
these to include an igt and have at least some hopes of a cross-driver
standard. Also the crc interface is all in debugfs, that's where this
belongs.
Please fix this before we ship it. Ideally we'd make this a standard part
so it can be used in igt testcase, but quick fix would be to either revert
or at least move into debugfs files (we have per-crtc files, so not hard
to pull off).
If this is for functional safety or whatever the IVI standard for that
was, then it needs real uapi treatment.
Thanks, Daniel
> +{
> + dm->crc_win_x_start_property =
> + drm_property_create_range(adev_to_drm(dm->adev),
> + DRM_MODE_PROP_ATOMIC,
> + "AMD_CRC_WIN_X_START", 0, U16_MAX);
> + if (!dm->crc_win_x_start_property)
> + return -ENOMEM;
> +
> + dm->crc_win_y_start_property =
> + drm_property_create_range(adev_to_drm(dm->adev),
> + DRM_MODE_PROP_ATOMIC,
> + "AMD_CRC_WIN_Y_START", 0, U16_MAX);
> + if (!dm->crc_win_y_start_property)
> + return -ENOMEM;
> +
> + dm->crc_win_x_end_property =
> + drm_property_create_range(adev_to_drm(dm->adev),
> + DRM_MODE_PROP_ATOMIC,
> + "AMD_CRC_WIN_X_END", 0, U16_MAX);
> + if (!dm->crc_win_x_end_property)
> + return -ENOMEM;
> +
> + dm->crc_win_y_end_property =
> + drm_property_create_range(adev_to_drm(dm->adev),
> + DRM_MODE_PROP_ATOMIC,
> + "AMD_CRC_WIN_Y_END", 0, U16_MAX);
> + if (!dm->crc_win_y_end_property)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +#endif
> +
> static int amdgpu_dm_init(struct amdgpu_device *adev)
> {
> struct dc_init_data init_data;
> @@ -1084,6 +1119,10 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>
> dc_init_callbacks(adev->dm.dc, &init_params);
> }
> +#endif
> +#ifdef CONFIG_DEBUG_FS
> + if (create_crtc_crc_properties(&adev->dm))
> + DRM_ERROR("amdgpu: failed to create crc property.\n");
> #endif
> if (amdgpu_dm_initialize_drm_device(adev)) {
> DRM_ERROR(
> @@ -5409,12 +5448,64 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> state->crc_src = cur->crc_src;
> state->cm_has_degamma = cur->cm_has_degamma;
> state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb;
> -
> +#ifdef CONFIG_DEBUG_FS
> + state->crc_window = cur->crc_window;
> +#endif
> /* TODO Duplicate dc_stream after objects are stream object is
> flattened */
>
> return &state->base;
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state,
> + struct drm_property *property,
> + uint64_t val)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct amdgpu_device *adev = drm_to_adev(dev);
> + struct dm_crtc_state *dm_new_state =
> + to_dm_crtc_state(crtc_state);
> +
> + if (property == adev->dm.crc_win_x_start_property)
> + dm_new_state->crc_window.x_start = val;
> + else if (property == adev->dm.crc_win_y_start_property)
> + dm_new_state->crc_window.y_start = val;
> + else if (property == adev->dm.crc_win_x_end_property)
> + dm_new_state->crc_window.x_end = val;
> + else if (property == adev->dm.crc_win_y_end_property)
> + dm_new_state->crc_window.y_end = val;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
> + const struct drm_crtc_state *state,
> + struct drm_property *property,
> + uint64_t *val)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct amdgpu_device *adev = drm_to_adev(dev);
> + struct dm_crtc_state *dm_state =
> + to_dm_crtc_state(state);
> +
> + if (property == adev->dm.crc_win_x_start_property)
> + *val = dm_state->crc_window.x_start;
> + else if (property == adev->dm.crc_win_y_start_property)
> + *val = dm_state->crc_window.y_start;
> + else if (property == adev->dm.crc_win_x_end_property)
> + *val = dm_state->crc_window.x_end;
> + else if (property == adev->dm.crc_win_y_end_property)
> + *val = dm_state->crc_window.y_end;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
> +
> static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> {
> enum dc_irq_source irq_source;
> @@ -5481,6 +5572,10 @@ static const struct drm_crtc_funcs
> amdgpu_dm_crtc_funcs = {
> .enable_vblank = dm_enable_vblank,
> .disable_vblank = dm_disable_vblank,
> .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> +#ifdef CONFIG_DEBUG_FS
> + .atomic_set_property = amdgpu_dm_crtc_atomic_set_property,
> + .atomic_get_property = amdgpu_dm_crtc_atomic_get_property,
> +#endif
> };
>
> static enum drm_connector_status
> @@ -6689,6 +6784,25 @@ static int amdgpu_dm_plane_init(struct
> amdgpu_display_manager *dm,
> return 0;
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +static void attach_crtc_crc_properties(struct amdgpu_display_manager *dm,
> + struct amdgpu_crtc *acrtc)
> +{
> + drm_object_attach_property(&acrtc->base.base,
> + dm->crc_win_x_start_property,
> + 0);
> + drm_object_attach_property(&acrtc->base.base,
> + dm->crc_win_y_start_property,
> + 0);
> + drm_object_attach_property(&acrtc->base.base,
> + dm->crc_win_x_end_property,
> + 0);
> + drm_object_attach_property(&acrtc->base.base,
> + dm->crc_win_y_end_property,
> + 0);
> +}
> +#endif
> +
> static int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
> struct drm_plane *plane,
> uint32_t crtc_index)
> @@ -6736,7 +6850,9 @@ static int amdgpu_dm_crtc_init(struct
> amdgpu_display_manager *dm,
> drm_crtc_enable_color_mgmt(&acrtc->base, MAX_COLOR_LUT_ENTRIES,
> true, MAX_COLOR_LUT_ENTRIES);
> drm_mode_crtc_set_gamma_size(&acrtc->base,
> MAX_COLOR_LEGACY_LUT_ENTRIES);
> -
> +#ifdef CONFIG_DEBUG_FS
> + attach_crtc_crc_properties(dm, acrtc);
> +#endif
> return 0;
>
> fail:
> @@ -8363,6 +8479,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
> drm_atomic_state *state)
> */
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> + bool configure_crc = false;
>
> dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>
> @@ -8372,21 +8489,30 @@ static void amdgpu_dm_atomic_commit_tail(struct
> drm_atomic_state *state)
> dc_stream_retain(dm_new_crtc_state->stream);
> acrtc->dm_irq_params.stream = dm_new_crtc_state->stream;
> manage_dm_interrupts(adev, acrtc, true);
> -
> + }
> #ifdef CONFIG_DEBUG_FS
> + if (new_crtc_state->active &&
> +
> amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src)) {
> /**
> * Frontend may have changed so reapply the CRC capture
> * settings for the stream.
> */
> dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> + dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
>
> - if
> (amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src)) {
> - amdgpu_dm_crtc_configure_crc_source(
> - crtc, dm_new_crtc_state,
> - dm_new_crtc_state->crc_src);
> + if (amdgpu_dm_crc_window_is_default(dm_new_crtc_state))
> {
> + if (!old_crtc_state->active ||
> drm_atomic_crtc_needs_modeset(new_crtc_state))
> + configure_crc = true;
> + } else {
> + if
> (amdgpu_dm_crc_window_changed(dm_new_crtc_state, dm_old_crtc_state))
> + configure_crc = true;
> }
> -#endif
> +
> + if (configure_crc)
> + amdgpu_dm_crtc_configure_crc_source(
> + crtc, dm_new_crtc_state,
> dm_new_crtc_state->crc_src);
> }
> +#endif
> }
>
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 963a69877455..f2aebbe4d140 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -336,6 +336,13 @@ struct amdgpu_display_manager {
> */
> const struct gpu_info_soc_bounding_box_v1_0 *soc_bounding_box;
>
> +#ifdef CONFIG_DEBUG_FS
> + /* set the crc calculation window*/
> + struct drm_property *crc_win_x_start_property;
> + struct drm_property *crc_win_y_start_property;
> + struct drm_property *crc_win_x_end_property;
> + struct drm_property *crc_win_y_end_property;
> +#endif
> /**
> * @mst_encoders:
> *
> @@ -422,6 +429,15 @@ struct dm_plane_state {
> struct dc_plane_state *dc_state;
> };
>
> +#ifdef CONFIG_DEBUG_FS
> +struct crc_rec {
> + uint16_t x_start;
> + uint16_t y_start;
> + uint16_t x_end;
> + uint16_t y_end;
> + };
> +#endif
> +
> struct dm_crtc_state {
> struct drm_crtc_state base;
> struct dc_stream_state *stream;
> @@ -444,6 +460,9 @@ struct dm_crtc_state {
> struct dc_info_packet vrr_infopacket;
>
> int abm_level;
> +#ifdef CONFIG_DEBUG_FS
> + struct crc_rec crc_window;
> +#endif
> };
>
> #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index c29dc11619f7..ff6db26626ea 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -81,6 +81,33 @@ const char *const *amdgpu_dm_crtc_get_crc_sources(struct
> drm_crtc *crtc,
> return pipe_crc_sources;
> }
>
> +bool amdgpu_dm_crc_window_is_default(struct dm_crtc_state *dm_crtc_state)
> +{
> + bool ret = true;
> +
> + if ((dm_crtc_state->crc_window.x_start != 0) ||
> + (dm_crtc_state->crc_window.y_start != 0) ||
> + (dm_crtc_state->crc_window.x_end != 0) ||
> + (dm_crtc_state->crc_window.y_end != 0))
> + ret = false;
> +
> + return ret;
> +}
> +
> +bool amdgpu_dm_crc_window_changed(struct dm_crtc_state *dm_new_crtc_state,
> + struct dm_crtc_state *dm_old_crtc_state)
> +{
> + bool ret = false;
> +
> + if ((dm_new_crtc_state->crc_window.x_start !=
> dm_old_crtc_state->crc_window.x_start) ||
> + (dm_new_crtc_state->crc_window.y_start !=
> dm_old_crtc_state->crc_window.y_start) ||
> + (dm_new_crtc_state->crc_window.x_end !=
> dm_old_crtc_state->crc_window.x_end) ||
> + (dm_new_crtc_state->crc_window.y_end !=
> dm_old_crtc_state->crc_window.y_end))
> + ret = true;
> +
> + return ret;
> +}
> +
> int
> amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
> size_t *values_cnt)
> @@ -105,6 +132,7 @@ int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc
> *crtc,
> struct dc_stream_state *stream_state = dm_crtc_state->stream;
> bool enable = amdgpu_dm_is_valid_crc_source(source);
> int ret = 0;
> + struct crc_params *crc_window = NULL, tmp_window;
>
> /* Configuration will be deferred to stream enable. */
> if (!stream_state)
> @@ -114,8 +142,21 @@ int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc
> *crtc,
>
> /* Enable CRTC CRC generation if necessary. */
> if (dm_is_crc_source_crtc(source)) {
> + if (!amdgpu_dm_crc_window_is_default(dm_crtc_state)) {
> + crc_window = &tmp_window;
> +
> + tmp_window.windowa_x_start =
> dm_crtc_state->crc_window.x_start;
> + tmp_window.windowa_y_start =
> dm_crtc_state->crc_window.y_start;
> + tmp_window.windowa_x_end =
> dm_crtc_state->crc_window.x_end;
> + tmp_window.windowa_y_end =
> dm_crtc_state->crc_window.y_end;
> + tmp_window.windowb_x_start =
> dm_crtc_state->crc_window.x_start;
> + tmp_window.windowb_y_start =
> dm_crtc_state->crc_window.y_start;
> + tmp_window.windowb_x_end =
> dm_crtc_state->crc_window.x_end;
> + tmp_window.windowb_y_end =
> dm_crtc_state->crc_window.y_end;
> + }
> +
> if (!dc_stream_configure_crc(stream_state->ctx->dc,
> - stream_state, NULL, enable,
> enable)) {
> + stream_state, crc_window, enable,
> enable)) {
> ret = -EINVAL;
> goto unlock;
> }
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
> index f7d731797d3f..0235bfb246e5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
> @@ -47,6 +47,9 @@ static inline bool amdgpu_dm_is_valid_crc_source(enum
> amdgpu_dm_pipe_crc_source
>
> /* amdgpu_dm_crc.c */
> #ifdef CONFIG_DEBUG_FS
> +bool amdgpu_dm_crc_window_is_default(struct dm_crtc_state *dm_crtc_state);
> +bool amdgpu_dm_crc_window_changed(struct dm_crtc_state *dm_new_crtc_state,
> + struct dm_crtc_state
> *dm_old_crtc_state);
> int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc *crtc,
> struct dm_crtc_state *dm_crtc_state,
> enum amdgpu_dm_pipe_crc_source source);
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index f522b664d3c6..5790affc7d61 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -3259,6 +3259,9 @@ void core_link_enable_stream(
> }
> }
>
> +#if defined(CONFIG_DRM_AMD_DC_DCN3_0)
> +#endif
> +
> dc->hwss.enable_audio_stream(pipe_ctx);
>
> /* turn off otg test pattern if enable */
> --
> 2.25.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-...@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
