Hi Shashank,

Thanks for the review. Please see my inline responses.

On Thu, 2020-12-10 at 18:07 +0530, Shashank Sharma wrote:
> Hello Aurabindo,
> 
> 
> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
> 
> > [Why&How]
> > If experimental freesync video mode module parameter is enabled,
> > add
> > few extra display modes into the driver's mode list corresponding
> > to common
> > video frame rates. When userspace sets these modes, no modeset will
> > be
> > performed (if current mode was one of freesync modes or the base
> > freesync mode
> > based off which timings have been generated for the rest of the
> > freesync modes)
> > since these modes only differ from the base mode with front porch
> > timing.
> > 
> > Signed-off-by: Aurabindo Pillai 
> > <aurabindo.pillai@amd.com>
> > 
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 197
> > ++++++++++++++++++
> >  1 file changed, 197 insertions(+)
> > 
> > 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 fbff8d693e03..f699a3d41cad 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -5178,6 +5178,69 @@ static void
> > dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
> >  	set_master_stream(context->streams, context->stream_count);
> >  }
> >  
> > +static struct drm_display_mode *
> > +get_highest_freesync_mode(struct amdgpu_dm_connector *aconnector,
> > +			  bool use_probed_modes)
>  the function name is confusing, this function has nothing to do with
> freesync. Probably drop the 'freesync' from the name,
> get_highest_refresh_rate_mode or similar ?

Will do.
>  
> > +{
> > +	struct drm_display_mode *m, *m_high = NULL;
> > +	u16 current_refresh, highest_refresh, preferred_mode_h,
> > preferred_mode_w;
> > +	struct list_head *list_head = use_probed_modes ?
> > +						    &aconnector-
> > >base.probed_modes :
> > +						    &aconnector-
> > >base.modes;
> > +
> > +	/*
> > +	 * Find the preferred mode
> > +	 */
>  Single line comment should be in /* */

Will do.
> > +
> > +	list_for_each_entry (m, list_head, head) {
> > +		if (!(m->type & DRM_MODE_TYPE_PREFERRED))
> > +			continue;
> > +
> > +		m_high = m;
> > +		preferred_mode_h = m_high->hdisplay;
> > +		preferred_mode_w = m_high->vdisplay;
> > +		highest_refresh = drm_mode_vrefresh(m_high);
> > +		break;
> > +	}
> > +
> > +	if (!m_high) {
> > +
> > +		/*
> > +		 * Probably an EDID with no preferred mode.
> > +		 * Fallback to first entry;
> > +		 */
> > +		m_high = list_first_entry_or_null(&aconnector-
> > >base.modes,
> > +						  struct
> > drm_display_mode, head);
> > +		if (!m_high)
> > +			return NULL;
> > +		else {
> > +			preferred_mode_h = m_high->hdisplay;
> > +			preferred_mode_w = m_high->vdisplay;
> > +			highest_refresh = drm_mode_vrefresh(m_high);
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * Find the mode with highest refresh rate with same
> > resolution.
> > +	 * For some monitors, preferred mode is not the mode with
> > highest
> > +	 * supported refresh rate.
> > +	 */
> > +	list_for_each_entry (m, list_head, head) {
> > +		current_refresh  = drm_mode_vrefresh(m);
> > +
> > +		if (m->hdisplay == preferred_mode_h &&
> > +		    m->vdisplay == preferred_mode_w &&
> > +		    highest_refresh < current_refresh) {
> > +			highest_refresh = current_refresh;
> > +			preferred_mode_h = m->hdisplay;
> > +			preferred_mode_w = m->vdisplay;
>  why do we need to save preferred_mode_h and w variables at all ? I
> thought the idea here was to get the mode with highest refresh rate,
> but same resolution, which is indicated in the if (cond) above also.
> I think we just need highest refresh rate isn't ? 

Yes you're right. Will make that change.

> > +			m_high = m;
> > +		}
> > +	}
> > +
> > +	return m_high;
> > +}
> > +
> >  static struct dc_stream_state *
> >  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> >  		       const struct drm_display_mode *drm_mode,
> > @@ -7006,6 +7069,139 @@ static void
> > amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +static bool is_duplicate_mode(struct amdgpu_dm_connector
> > *aconnector,
> > +			      struct drm_display_mode *mode)
> > +{
> > +	struct drm_display_mode *m;
> > +
> > +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> > +		if (m->clock == mode->clock &&
> > +		    m->htotal == mode->htotal &&
> > +		    m->vtotal == mode->vtotal &&
> > +		    m->hdisplay == mode->hdisplay &&
> > +		    m->vdisplay == mode->vdisplay &&
> > +		    m->hsync_start == mode->hsync_start &&
> > +		    m->vsync_start == mode->vsync_start &&
> > +		    m->vsync_end == mode->vsync_end &&
> > +		    m->hsync_end == mode->hsync_end)
> > +			return true;
> > +	}
> > +
> > +	return false;
>  Why not use drm_mode_equal() instead ?

Wasnt aware of this API, thanks!

> > +}
> > +
> > +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> > +			 struct detailed_data_monitor_range *range)
> > +{
> > +	const struct drm_display_mode *m, *m_save;
> > +	struct drm_display_mode *new_mode;
> > +	uint i;
> > +	uint64_t target_vtotal, target_vtotal_diff;
> > +	uint32_t new_modes_count = 0;
> > +	uint64_t num, den;
> > +
> > +	/* Standard FPS values
> > +	 *
> > +	 * 23.976 - TV/NTSC
> > +	 * 24	  - Cinema
> > +	 * 25     - TV/PAL
> > +	 * 29.97  - TV/NTSC
> > +	 * 30     - TV/NTSC
> > +	 * 48	  - Cinema HFR
> > +	 * 50	  - TV/PAL
> > +	 */
> > +	const uint32_t neededrates[] = { 23976, 24000, 25000, 29970,
> > +					 30000, 48000, 50000, 72000,
> > 96000 };
> > +
> > +	/*
> > +	 * Find mode with highest refresh rate with the same resolution
> > +	 * as the preferred mode. Some monitors report a preferred mode
> > +	 * with lower resolution than the highest refresh rate
> > supported.
> > +	 */
> > +
> > +	m_save = get_highest_freesync_mode(aconnector, true);
>  A NULL return check here to bypass the whole routine below ? 

Will do.
> > +
> > +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> > +		if (m != m_save)
> > +			continue;
> > +
> > +		for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t);
> > i++) {
> > +			if (drm_mode_vrefresh(m) * 1000 <
> > neededrates[i])
> > +				continue;
> > +
> > +			if (neededrates[i] < range->min_vfreq * 1000)
> > +				continue;
> > +
> > +			num = (unsigned long long)m->clock * 1000 *
> > 1000;
>  
> > +			den = neededrates[i] * (unsigned long long)m-
> > >htotal;
> > +			target_vtotal = div_u64(num, den);
> > +			target_vtotal_diff = target_vtotal - m->vtotal;
> > +
> > +			/*
> > +			 * Check for illegal modes
> > +			 */
>  Same here for single line comment
> > +			if (m->vsync_start + target_vtotal_diff < m-
> > >vdisplay ||
> > +			    m->vsync_end + target_vtotal_diff < m-
> > >vsync_start ||
> > +			    m->vtotal + target_vtotal_diff < m-
> > >vsync_end)
> > +				continue;
> > +
> > +			new_mode = drm_mode_duplicate(aconnector-
> > >base.dev, m);
> > +			if (!new_mode)
> > +				goto out;
> > +
> > +			new_mode->vtotal += (u16)target_vtotal_diff;
> > +			new_mode->vsync_start +=
> > (u16)target_vtotal_diff;
> > +			new_mode->vsync_end += (u16)target_vtotal_diff;
> > +			new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
> > +			new_mode->type |= DRM_MODE_TYPE_DRIVER;
> > +			strcat(new_mode->name, "_FSV\0");
> > +
> > +			if (!is_duplicate_mode(aconnector, new_mode)) {
>  or drm_mode_equal here ?

Will modify is_duplicate_mode() to use drm_mode_equal.

> > +				drm_mode_probed_add(&aconnector->base,
> > new_mode);
> > +				new_modes_count += 1;
> > +			} else
> > +				drm_mode_destroy(aconnector->base.dev,
> > new_mode);
> > +		}
> > +	}
> > + out:
> > +	return new_modes_count;
> > +}
> > +
> > +static void amdgpu_dm_connector_add_freesync_modes(struct
> > drm_connector *connector,
> > +						   struct edid *edid)
> > +{
> > +	uint8_t i;
> > +	struct detailed_timing *timing;
> > +	struct detailed_non_pixel *data;
> > +	struct detailed_data_monitor_range *range;
> > +	struct amdgpu_dm_connector *amdgpu_dm_connector =
> > +		to_amdgpu_dm_connector(connector);
> > +
> > +	if (!(amdgpu_exp_freesync_vid_mode && edid))
> > +		return;
>  
> > +
> > +	if (!(amdgpu_dm_connector->dc_sink->sink_signal ==
> > SIGNAL_TYPE_EDP ||
> 
> do we want the freesync infra to be available for eDP also ? 
> 

Freesync is enabled on eDP as well. There is no reason to block it.
Only requirement would be to check if the monitor actually supports
VRR. So it makes sense to remove this check. We are already checking
for VRR support below through EDID_DETAIL_MONITOR_RANGE.


--

Regards,
Aurabindo Pillai