Re: [PATCH 13/17] drm/amd/display: Expose new CRC window property

On Fri, Nov 13, 2020 at 03:56:41PM -0500, Bindu Ramamurthy wrote:

From: Wayne Lin Wayne.Lin@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.Lin@amd.com Acked-by: Bindu Ramamurthy bindu.r@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-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx