[Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Harry Wentland <harry.wentland at amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor at intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Please no. A loop iterator called 'i' should always be a normal signed int.

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?

Aye, all typical clock frequencies should be in khz.

Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

-- 
Ville Syrjälä
Intel