[PATCH 3/3] drm/amd/display: Skip modeset for front porch change

Kazlauskas, Nicholas nicholas.kazlauskas at amd.com
Mon Feb 8 15:06:01 UTC 2021


On 2021-01-24 11:00 p.m., Aurabindo Pillai wrote:
> 
> 
> On 2021-01-21 2:05 p.m., Kazlauskas, Nicholas wrote:
>> On 2021-01-19 10:50 a.m., Aurabindo Pillai wrote:
>>> [Why]
>>> A seamless transition between modes can be performed if the new incoming
>>> mode has the same timing parameters as the optimized mode on a 
>>> display with a
>>> variable vtotal min/max.
>>>
>>> Smooth video playback usecases can be enabled with this seamless 
>>> transition by
>>> switching to a new mode which has a refresh rate matching the video.
>>>
>>> [How]
>>> Skip full modeset if userspace requested a compatible freesync mode 
>>> which only
>>> differs in the front porch timing from the current mode.
>>>
>>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
>>> Acked-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 233 +++++++++++++++---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
>>>   2 files changed, 198 insertions(+), 36 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 aaef2fb528fd..d66494cdd8c8 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);
>>>   /*
>>>    * 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;
>>> +    }
>>> +
>>
>> Not sure if I commented on this last time but I don't really 
>> understand what this is_in_modeset logic is supposed to be doing here.
> 
> This is so because create_stream_for_link() that ends up calling this 
> function has two callers, one which is for stream validation in which 
> the created stream is immediately discarded. The other is during 
> modeset. Depending on these two cases, we want to copy the right timing 
> parameters. With this method, major refactor wasn't necessary with the 
> upper layers.

I don't understand why the timing parameters would change between what 
we validated and what we're planning on applying to the hardware. I 
think we should be validating the same thing in both cases, especially 
if it's for something internal like DC stream timing.

> 
>>
>> We should be modifying crtc_vsync_* for the generated modes, no? Not 
>> just the vsync_* parameters.
> 
> This is already handled with:
> 
>      if (!dm_state)
>          drm_mode_set_crtcinfo(&mode, 0);
> 
>      if (dm_state && is_fs_vid_mode)
>          drm_mode_set_crtcinfo(&saved_mode, 0);
> 

OK, that's fine then.

>>
>>>       timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>>>       stream->output_color_space = get_output_color_space(timing_out);
>>> @@ -5227,6 +5240,33 @@ 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;
>>> +    int timing_diff;
>>> +
>>> +    high_mode = get_highest_refresh_rate_mode(aconnector, false);
>>> +    if (!high_mode || !mode)
>>> +        return false;
>>> +
>>> +    timing_diff = high_mode->vtotal - mode->vtotal;
>>> +
>>> +    if (high_mode->clock == 0 || high_mode->clock != mode->clock ||
>>> +        high_mode->hdisplay != mode->hdisplay ||
>>> +        high_mode->vdisplay != mode->vdisplay ||
>>> +        high_mode->hsync_start != mode->hsync_start ||
>>> +        high_mode->hsync_end != mode->hsync_end ||
>>> +        high_mode->htotal != mode->htotal ||
>>> +        high_mode->hskew != mode->hskew ||
>>> +        high_mode->vscan != mode->vscan ||
>>> +        high_mode->vsync_start - mode->vsync_start != timing_diff ||
>>> +        high_mode->vsync_end - mode->vsync_end != timing_diff)
>>> +        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 +5280,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 = false;
>>>   #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(saved_mode));
>>> +
>>>       if (aconnector == NULL) {
>>>           DRM_ERROR("aconnector is NULL!\n");
>>>           return stream;
>>> @@ -5301,25 +5347,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 = amdgpu_freesync_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;
>>> +        } else {
>>> +            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);
>>> +
>>> +
>>
>> Not sure what this above bit is doing here:
>>
>> (1) We're in atomic check and (2) It's a FS video mode
>>
>> -> set CRTC modesetting timing parameters?
>>
>> What's this for?
> 
> Without this change, kernel would tell the userspace we're sending one 
> of the freesync mode timing, but actually it would send the optimized 
> mode timing parameter. For instance, if 144Hz is the optimized mode, and 
> the user sets 48Hz, the display settings would reflect the change, but 
> the monitor would still see 144Hz.

Sorry, but I still don't understand why it depends on dm_state here. 
You're trying to avoid updating crtcinfo in mode validation for some reason?

>>
>>         /*
>>>       * 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);
>>
>> I don't see my feedback on previous patches addressed here - isn't it 
>> cleaner to just merge the is_scaling/is_fs_vid_mode checks here? The 
>> idea is that we're replacing the mode with the primary/preferred in 
>> both cases.
> 
> Sorry, I must have missed it. Will address this in the next iteration.
>>
>> Using dm_state as a check for validation / vs atomic check is 
>> questionable as well. In practice we're guaranteed to have it today 
>> but it might not be clear if there are future changes that it's being 
>> used for this purpose here if that changes.
> 
> This is because we're reusing this function for testing stream 
> validation by calling it directly with a NULL parameter for dm_state. If 
> this usage is not recommended, then a separate function can be used for 
> just stream validation and this usage of dm_state can be dropped.
> 

Like my comments above I'm not sure why these would differ between 
validation/commit time - they (ideally) should not.

>>
>>>       stream->timing.flags.DSC = 0;
>>> @@ -5456,6 +5516,7 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>>       state->abm_level = cur->abm_level;
>>>       state->vrr_supported = cur->vrr_supported;
>>>       state->freesync_config = cur->freesync_config;
>>> +    state->freesync_video_mode = cur->freesync_video_mode;
>>>       state->crc_src = cur->crc_src;
>>>       state->cm_has_degamma = cur->cm_has_degamma;
>>>       state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb;
>>> @@ -7149,7 +7210,7 @@ static void 
>>> amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connect
>>>       struct amdgpu_dm_connector *amdgpu_dm_connector =
>>>           to_amdgpu_dm_connector(connector);
>>> -    if (!(amdgpu_exp_freesync_vid_mode && edid))
>>> +    if (!(amdgpu_freesync_vid_mode && edid))
>>>           return;
>>>       if (edid->version == 1 && edid->revision > 1) {
>>> @@ -7847,9 +7908,25 @@ static void update_stream_irq_parameters(
>>>       if (new_crtc_state->vrr_supported &&
>>>           config.min_refresh_in_uhz &&
>>>           config.max_refresh_in_uhz) {
>>> -        config.state = new_crtc_state->base.vrr_enabled ?
>>> -            VRR_STATE_ACTIVE_VARIABLE :
>>> -            VRR_STATE_INACTIVE;
>>> +        /*
>>> +         * 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 &&
>>
>> This condition doesn't need to check max_refresh/min_refresh as it's 
>> already checked aboved.
> 
> Will fix.
>>
>>> +            (!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;
>>> +        } else {
>>> +            config.state = new_crtc_state->base.vrr_enabled ?
>>> +                             VRR_STATE_ACTIVE_VARIABLE :
>>> +                             VRR_STATE_INACTIVE;
>>> +        }
>>> +
>>>       } else {
>>>           config.state = VRR_STATE_UNSUPPORTED;
>>>       }
>>> @@ -8171,7 +8248,8 @@ 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_video_mode) {
>>
>> Do we really need this check here? amdgpu_dm_vrr_active should pick up 
>> on VRR_STATE_ACTIVE_FIXED, no? If it doesn't then that should be fixed.
> 
> This specifically checks for a change the current and previous state. In 
> both the states, freesync_video_mode can be active. This condition would 
> return false in the absence of additional OR with freesync_video_mode. 
> If freesync_video_mode is active, we want this block of code to get 
> executed. If freesync_video_mode is not active, then comparison with old 
> and new state is sufficient. I can add this logic into another function 
> and keep it cleaner here.
> 

I think acrtc_state->freesync_video_mode is redundant - it doesn't check 
prev/new, it only checks whether or not it's active. This should be 
already checked in amdgpu_dm_vrr_active(), and the:

amdgpu_dm_vrr_active(dm_old_crtc_state) != amdgpu_dm_vrr_active(acrtc_state)

should be doing this for you.

>>
>>>               spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>>>               dc_stream_adjust_vmin_vmax(
>>>                   dm->dc, acrtc_state->stream,
>>> @@ -8442,8 +8520,10 @@ static void 
>>> amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>>                   continue;
>>>               }
>>> -            if (dm_old_crtc_state->stream)
>>> +            if (dm_old_crtc_state->stream) {
>>>                   remove_stream(adev, acrtc, dm_old_crtc_state->stream);
>>> +                dm_old_crtc_state->freesync_video_mode = 0;
>>> +            }
>>>               pm_runtime_get_noresume(dev->dev);
>>> @@ -8454,8 +8534,10 @@ static void 
>>> amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>>           } else if (modereset_required(new_crtc_state)) {
>>>               DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id 
>>> %d:[%p]\n", acrtc->crtc_id, acrtc);
>>>               /* i.e. reset mode */
>>> -            if (dm_old_crtc_state->stream)
>>> +            if (dm_old_crtc_state->stream) {
>>>                   remove_stream(adev, acrtc, dm_old_crtc_state->stream);
>>> +                dm_old_crtc_state->freesync_video_mode = 0;
>>> +            }
>>>               mode_set_reset_required = true;
>>>           }
>>>       } /* for_each_crtc_in_state() */
>>> @@ -8867,6 +8949,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 &&
>>> @@ -8874,17 +8957,25 @@ 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) {
>>> +            config.state = VRR_STATE_ACTIVE_VARIABLE;
>>> +        } else {
>>> +            config.state = VRR_STATE_INACTIVE;
>>> +        }
>>> +    }
>>> +out:
>>>       new_crtc_state->freesync_config = config;
>>>   }
>>> @@ -8897,6 +8988,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,
>>> @@ -8987,6 +9123,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 (amdgpu_freesync_vid_mode &&
>>> +            dm_new_crtc_state->stream &&
>>> +            is_timing_unchanged_for_freesync(new_crtc_state, 
>>> old_crtc_state))
>>> +            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)) {
>>> @@ -9018,6 +9159,26 @@ static int dm_update_crtc_state(struct 
>>> amdgpu_display_manager *dm,
>>>           if (!dm_old_crtc_state->stream)
>>>               goto skip_modeset;
>>> +        if (amdgpu_freesync_vid_mode && dm_new_crtc_state->stream &&
>>> +            is_timing_unchanged_for_freesync(new_crtc_state,
>>> +                             old_crtc_state)) {
>>> +            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 (amdgpu_freesync_vid_mode && aconnector &&
>>> +               is_freesync_video_mode(&new_crtc_state->mode,
>>> +                          aconnector)) {
>>> +            set_freesync_fixed_config(dm_new_crtc_state);
>>
>> I don't think this extra check is needed here because we have the one 
>> above.
> 
> In the second branch, since we call is_freesync_video_mode(), the check 
> with the module parameter was also necessary as this is the triggering 
> point for the VRR api configuration change. Also, the first branch skips 
> the modeset, while the second doesnt. So we cannot club them together. 
> The second branch is for the case when a freesync mode was set 
> accompanied by a modeset. This happens when the previous mode had 
> different base timings and hence a modeset is necessary.
> 
> 
> Thanks & Regards,
> Aurabindo Pillai
> 

Okay, I understand this block now. Thanks.

Regards,
Nicholas Kazlauskas

>>
>> Regards,
>> Nicholas Kazlauskas
>>
>>> +        } else if (dm_new_crtc_state->freesync_video_mode) {
>>> +            dm_new_crtc_state->freesync_video_mode = 0;
>>> +        }
>>> +
>>>           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 3ea85be9c546..ff4675572125 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -474,6 +474,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;
>>>
>>
> 




More information about the dri-devel mailing list