On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@xxxxxxx>
> Acked-by: Christian König <christian.koenig at amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 210 +++++++++++++++---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 179 insertions(+), 32 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 c1ffd33e9d83..4ba6be59dcd5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
Do we need this forward declaration for a static function ?
> /*
> * dm_vblank_get_counter
> *
> @@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
> const struct drm_connector *connector,
> const struct drm_connector_state *connector_state,
> const struct dc_stream_state *old_stream,
> - int requested_bpc)
> + int requested_bpc,
> + bool is_in_modeset)
> {
> struct dc_crtc_timing *timing_out = &stream->timing;
> const struct drm_display_info *info = &connector->display_info;
> @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
> timing_out->hdmi_vic = hv_frame.vic;
> }
>
> - timing_out->h_addressable = mode_in->crtc_hdisplay;
> - timing_out->h_total = mode_in->crtc_htotal;
> - timing_out->h_sync_width =
> - mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> - timing_out->h_front_porch =
> - mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> - timing_out->v_total = mode_in->crtc_vtotal;
> - timing_out->v_addressable = mode_in->crtc_vdisplay;
> - timing_out->v_front_porch =
> - mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> - timing_out->v_sync_width =
> - mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> - timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> + if (is_in_modeset) {
> + timing_out->h_addressable = mode_in->hdisplay;
> + timing_out->h_total = mode_in->htotal;
> + timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
> + timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
> + timing_out->v_total = mode_in->vtotal;
> + timing_out->v_addressable = mode_in->vdisplay;
> + timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
> + timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
> + timing_out->pix_clk_100hz = mode_in->clock * 10;
> + } else {
> + timing_out->h_addressable = mode_in->crtc_hdisplay;
> + timing_out->h_total = mode_in->crtc_htotal;
> + timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> + timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> + timing_out->v_total = mode_in->crtc_vtotal;
> + timing_out->v_addressable = mode_in->crtc_vdisplay;
> + timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> + timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> + timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> + }
> +
> timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>
> stream->output_color_space = get_output_color_space(timing_out);
> @@ -5227,6 +5240,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
> return m_pref;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> + struct amdgpu_dm_connector *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> +
> + high_mode = get_highest_refresh_rate_mode(aconnector, false);
> + if (!high_mode || !mode)
> + return false;
> +
> + if (high_mode->clock == 0 ||
> + high_mode->hdisplay != mode->hdisplay ||
> + high_mode->vdisplay != mode->vdisplay ||
> + high_mode->clock != mode->clock)
> + return false;
> + else
> + return true;
> +}
> +
> static struct dc_stream_state *
> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_display_mode *drm_mode,
> @@ -5240,15 +5271,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> dm_state ? &dm_state->base : NULL;
> struct dc_stream_state *stream = NULL;
> struct drm_display_mode mode = *drm_mode;
> + struct drm_display_mode saved_mode;
> + struct drm_display_mode *freesync_mode = NULL;
> bool native_mode_found = false;
> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
> int mode_refresh;
> int preferred_refresh = 0;
> + bool is_fs_vid_mode = 0;
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> struct dsc_dec_dpcd_caps dsc_caps;
> uint32_t link_bandwidth_kbps;
> #endif
> struct dc_sink *sink = NULL;
> +
> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> +
> if (aconnector == NULL) {
> DRM_ERROR("aconnector is NULL!\n");
> return stream;
> @@ -5301,25 +5338,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> */
> DRM_DEBUG_DRIVER("No preferred mode found\n");
> } else {
> - decide_crtc_timing_for_drm_display_mode(
> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> + if (is_fs_vid_mode) {
> + freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
> + saved_mode = mode;
> + mode = *freesync_mode;
> + }
> +
> + if (!is_fs_vid_mode)
This should go as else {} case for above if() instead of a new check
> + decide_crtc_timing_for_drm_display_mode(
> &mode, preferred_mode,
> dm_state ? (dm_state->scaling != RMX_OFF) : false);
> +
> preferred_refresh = drm_mode_vrefresh(preferred_mode);
> }
>
> if (!dm_state)
> drm_mode_set_crtcinfo(&mode, 0);
>
> - /*
> + if (dm_state && is_fs_vid_mode)
> + drm_mode_set_crtcinfo(&saved_mode, 0);
Why is this step required ?
> +
> + /*
> * If scaling is enabled and refresh rate didn't change
> * we copy the vic and polarities of the old timings
> */
> - if (!scale || mode_refresh != preferred_refresh)
> - fill_stream_properties_from_drm_display_mode(stream,
> - &mode, &aconnector->base, con_state, NULL, requested_bpc);
> + if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
> + fill_stream_properties_from_drm_display_mode(
> + stream, &mode, &aconnector->base, con_state, NULL,
> + requested_bpc, dm_state ? 1 : 0);
> else
> - fill_stream_properties_from_drm_display_mode(stream,
> - &mode, &aconnector->base, con_state, old_stream, requested_bpc);
> + fill_stream_properties_from_drm_display_mode(
> + stream, &mode, &aconnector->base, con_state, old_stream,
> + requested_bpc, dm_state ? 1 : 0);
>
> stream->timing.flags.DSC = 0;
>
> @@ -7844,13 +7895,29 @@ static void update_stream_irq_parameters(
> if (new_crtc_state->vrr_supported &&
> config.min_refresh_in_uhz &&
> config.max_refresh_in_uhz) {
> + /*
> + * if freesync compatible mode was set, config.state will be set
> + * in atomic check
> + */
> + if (config.state == VRR_STATE_ACTIVE_FIXED &&
> + config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
> + config.min_refresh_in_uhz &&
> + (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> + new_crtc_state->freesync_video_mode)) {
> + vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
> + vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
> + vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
> + vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> + goto out;
> + }
> +
> config.state = new_crtc_state->base.vrr_enabled ?
> VRR_STATE_ACTIVE_VARIABLE :
> VRR_STATE_INACTIVE;
> - } else {
> + } else
conventionally, if a if() condition has {}, its else condition should also have {}, even if it's a single statement. So you can skip this change.
> config.state = VRR_STATE_UNSUPPORTED;
> - }
>
> +out:
> mod_freesync_build_vrr_params(dm->freesync_module,
> new_stream,
> &config, &vrr_params);
> @@ -8168,7 +8235,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> * as part of commit.
> */
> if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> - amdgpu_dm_vrr_active(acrtc_state)) {
> + amdgpu_dm_vrr_active(acrtc_state) ||
> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + acrtc_state->freesync_video_mode) {
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> dc_stream_adjust_vmin_vmax(
> dm->dc, acrtc_state->stream,
> @@ -8864,6 +8933,7 @@ static void get_freesync_config_for_crtc(
> to_amdgpu_dm_connector(new_con_state->base.connector);
> struct drm_display_mode *mode = &new_crtc_state->base.mode;
> int vrefresh = drm_mode_vrefresh(mode);
> + bool fs_vid_mode = false;
>
> new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
> vrefresh >= aconnector->min_vfreq &&
> @@ -8871,17 +8941,26 @@ static void get_freesync_config_for_crtc(
>
> if (new_crtc_state->vrr_supported) {
> new_crtc_state->stream->ignore_msa_timing_param = true;
> - config.state = new_crtc_state->base.vrr_enabled ?
> - VRR_STATE_ACTIVE_VARIABLE :
> - VRR_STATE_INACTIVE;
> - config.min_refresh_in_uhz =
> - aconnector->min_vfreq * 1000000;
> - config.max_refresh_in_uhz =
> - aconnector->max_vfreq * 1000000;
> + fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + new_crtc_state->freesync_video_mode;
> +
> + config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
> + config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
> config.vsif_supported = true;
> config.btr = true;
> - }
>
> + if (fs_vid_mode) {
> + config.state = VRR_STATE_ACTIVE_FIXED;
> + config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> + goto out;
> + }
> + else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
> + config.state = VRR_STATE_ACTIVE_VARIABLE;
> + else
> + config.state = VRR_STATE_INACTIVE;
> +
> + }
> +out:
> new_crtc_state->freesync_config = config;
> }
>
> @@ -8894,6 +8973,51 @@ static void reset_freesync_config_for_crtc(
> sizeof(new_crtc_state->vrr_infopacket));
> }
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state)
> +{
> + struct drm_display_mode old_mode, new_mode;
> +
> + if (!old_crtc_state || !new_crtc_state)
> + return false;
> +
> + old_mode = old_crtc_state->mode;
> + new_mode = new_crtc_state->mode;
> +
> + if (old_mode.clock == new_mode.clock &&
> + old_mode.hdisplay == new_mode.hdisplay &&
> + old_mode.vdisplay == new_mode.vdisplay &&
> + old_mode.htotal == new_mode.htotal &&
> + old_mode.vtotal != new_mode.vtotal &&
> + old_mode.hsync_start == new_mode.hsync_start &&
> + old_mode.vsync_start != new_mode.vsync_start &&
> + old_mode.hsync_end == new_mode.hsync_end &&
> + old_mode.vsync_end != new_mode.vsync_end &&
> + old_mode.hskew == new_mode.hskew &&
> + old_mode.vscan == new_mode.vscan &&
> + (old_mode.vsync_end - old_mode.vsync_start) ==
> + (new_mode.vsync_end - new_mode.vsync_start))
> + return true;
> +
> + return false;
> +}
> +
> +static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
> + uint64_t num, den, res;
> + struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
> +
> + dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
> +
> + num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
> + den = (unsigned long long)new_crtc_state->mode.htotal *
> + (unsigned long long)new_crtc_state->mode.vtotal;
> +
> + res = div_u64(num, den);
> + dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
> + dm_new_crtc_state->freesync_video_mode = true;
> +}
> +
> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_atomic_state *state,
> struct drm_crtc *crtc,
> @@ -8984,6 +9108,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> * TODO: Refactor this function to allow this check to work
> * in all conditions.
> */
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode)
keeping this flag check before the function call might allow the compiler to save the function call, when this flag is disabled.
> + goto skip_modeset;
> +
> if (dm_new_crtc_state->stream &&
> dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> @@ -9015,6 +9144,23 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> if (!dm_old_crtc_state->stream)
> goto skip_modeset;
>
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode) {
same here
> + new_crtc_state->mode_changed = false;
> + DRM_DEBUG_DRIVER(
> + "Mode change not required for front porch change, "
> + "setting mode_changed to %d",
> + new_crtc_state->mode_changed);
> +
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> + goto skip_modeset;
> + } else if (aconnector &&
> + is_freesync_video_mode(&new_crtc_state->mode, aconnector) &&
> + amdgpu_exp_freesync_vid_mode)
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> ret = dm_atomic_get_state(state, &dm_state);
> if (ret)
> goto fail;
> 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 f5a5f1ad7dba..fd5fb9dafded 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -455,6 +455,7 @@ struct dm_crtc_state {
>
> bool freesync_timing_changed;
> bool freesync_vrr_info_changed;
> + bool freesync_video_mode;
>
> bool dsc_force_changed;
> bool vrr_supported;
With the minor comments fixed/explained above,
this patch is Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxx>
- Shashank
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
