Message-ID: <20171211101951.fh6mf3bjbtoqo5jz@phenom.ffwll.local>
Date:   Mon, 11 Dec 2017 11:19:51 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Keith Packard <keithp@...thp.com>
Cc:     linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm: Update edid-derived drm_display_info fields at edid
 property set

On Thu, Dec 07, 2017 at 04:42:57PM -0800, Keith Packard wrote:
> There are a set of values in the drm_display_info structure for each
> connector which hold information derived from EDID. These are computed
> in drm_add_display_info. Before this patch, that was only called in
> drm_add_edid_modes. This meant that they were only set when EDID was
> present and never reset when EDID was not, as happened when the
> display was disconnected.
> 
> One of these fields, non_desktop, is used from
> drm_mode_connector_update_edid_property, the function responsible for
> assigning the new edid value to the application-visible property.
> 
> Various drivers call these two functions (drm_add_edid_modes and
> drm_mode_connector_update_edid_property) in different orders. This
> means that even when EDID is present, the drm_display_info fields may
> not have been computed at the time that
> drm_mode_connector_update_edid_property used the non_desktop value to
> set the non_desktop property.
> 
> I've added a public function (drm_reset_display_info) that resets the
> drm_display_info field values to default values and then made the
> drm_add_display_info function public. These two functions are now
> called directly from drm_mode_connector_update_edid_property so that
> the drm_display_info fields are always computed from the current EDID
> information before being used in that function.
> 
> This means that the drm_display_info values are often computed twice,
> once when the EDID property it set and a second time when EDID is used
> to compute modes for the device. The alternative would be to uniformly
> ensure that the values were computed once before being used, which
> would require that all drivers reliably invoke the two paths in the
> same order. The computation is inexpensive enough that it seems more
> maintainable in the long term to simply compute them in both paths.
> 
> The API to drm_add_display_info has been changed so that it no longer
> takes the set of edid-based quirks as a parameter. Rather, it now
> computes those quirks itself and returns them for further use by
> drm_add_edid_modes.
> 
> This patch also includes a number of 'const' additions caused by
> drm_mode_connector_update_edid_property taking a 'const struct edid *'
> parameter and wanting to pass that along to drm_add_display_info.
> 
> Signed-off-by: Keith Packard <keithp@...thp.com>

Yeah looks like a good way to reset the desktop-mode in all cases.

> ---
>  drivers/gpu/drm/drm_connector.c | 13 ++++++++++
>  drivers/gpu/drm/drm_edid.c      | 53 ++++++++++++++++++++++++++++++-----------
>  include/drm/drm_edid.h          |  2 ++
>  3 files changed, 54 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 25f4b2e9a44f..83c01f359d55 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1207,6 +1207,19 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  	if (edid)
>  		size = EDID_LENGTH * (1 + edid->extensions);
>  
> +	/* Set the display info, using edid if available, otherwise
> +	 * reseting the values to defaults. This duplicates the work
> +	 * done in drm_add_edid_modes, but that function is not
> +	 * consistently called before this one in all drivers and the
> +	 * computation is cheap enough that it seems better to
> +	 * duplicate it rather than attempt to ensure some arbitrary
> +	 * ordering of calls.
> +	 */

Looks like the most reasonable fix for -rc. For the future, maybe capture
a FIXME/TODO in this comment that we should look into merging
drm_add_edid_modes() and drm_mode_connector_update_edid_property(). I
really can't think of a good reason for that split.

> +	if (edid)
> +		drm_add_display_info(connector, edid);
> +	else
> +		drm_reset_display_info(connector);
> +
>  	drm_object_property_set_value(&connector->base,
>  				      dev->mode_config.non_desktop_property,
>  				      connector->display_info.non_desktop);
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 524eace3d460..365901c1c33c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1731,7 +1731,7 @@ EXPORT_SYMBOL(drm_edid_duplicate);
>   *
>   * Returns true if @vendor is in @edid, false otherwise
>   */
> -static bool edid_vendor(struct edid *edid, const char *vendor)
> +static bool edid_vendor(const struct edid *edid, const char *vendor)
>  {
>  	char edid_vendor[3];
>  
> @@ -1749,7 +1749,7 @@ static bool edid_vendor(struct edid *edid, const char *vendor)
>   *
>   * This tells subsequent routines what fixes they need to apply.
>   */
> -static u32 edid_get_quirks(struct edid *edid)
> +static u32 edid_get_quirks(const struct edid *edid)
>  {
>  	const struct edid_quirk *quirk;
>  	int i;
> @@ -2813,7 +2813,7 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  /*
>   * Search EDID for CEA extension block.
>   */
> -static u8 *drm_find_edid_extension(struct edid *edid, int ext_id)
> +static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -2835,12 +2835,12 @@ static u8 *drm_find_edid_extension(struct edid *edid, int ext_id)
>  	return edid_ext;
>  }
>  
> -static u8 *drm_find_cea_extension(struct edid *edid)
> +static u8 *drm_find_cea_extension(const struct edid *edid)
>  {
>  	return drm_find_edid_extension(edid, CEA_EXT);
>  }
>  
> -static u8 *drm_find_displayid_extension(struct edid *edid)
> +static u8 *drm_find_displayid_extension(const struct edid *edid)
>  {
>  	return drm_find_edid_extension(edid, DISPLAYID_EXT);
>  }
> @@ -4378,7 +4378,7 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
>  }
>  
>  static void drm_parse_cea_ext(struct drm_connector *connector,
> -			      struct edid *edid)
> +			      const struct edid *edid)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  	const u8 *edid_ext;
> @@ -4412,11 +4412,34 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> -static void drm_add_display_info(struct drm_connector *connector,
> -				 struct edid *edid, u32 quirks)
> +/* 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
> + */
> +void
> +drm_reset_display_info(struct drm_connector *connector)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +
> +	info->width_mm = 0;
> +	info->height_mm = 0;
> +
> +	info->bpc = 0;
> +	info->color_formats = 0;
> +	info->cea_rev = 0;
> +	info->max_tmds_clock = 0;
> +	info->dvi_dual = false;
> +	info->has_hdmi_infoframe = false;
> +
> +	info->non_desktop = 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_reset_display_info);

Do we really need these 2 EXPORT_SYMBOL here?

>From a quick look the users are only internal to drm.ko, so not needed.
And that would gives us a clearer driver api (plus I won't nag you about
lack of kerneldoc for said driver api).

If that's correct pls also move the decls to drm_crtc_internal.h. There's
already another function exported like that from drm_edid.c.

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>

Please also cc: intel-gfx for v2 so our CI can double-check I didn't miss
anything.

Dave's out, but there's some other drm-misc fixes, I'll probably forward a
-fixes pull to Linus directly.

Thanks, Daniel

> +
> +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  
> +	u32 quirks = edid_get_quirks(edid);
> +
>  	info->width_mm = edid->width_cm * 10;
>  	info->height_mm = edid->height_cm * 10;
>  
> @@ -4430,11 +4453,13 @@ static void drm_add_display_info(struct drm_connector *connector,
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> +
>  	if (edid->revision < 3)
> -		return;
> +		return quirks;
>  
>  	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
> -		return;
> +		return quirks;
>  
>  	drm_parse_cea_ext(connector, edid);
>  
> @@ -4454,7 +4479,7 @@ static void drm_add_display_info(struct drm_connector *connector,
>  
>  	/* Only defined for 1.4 with digital displays */
>  	if (edid->revision < 4)
> -		return;
> +		return quirks;
>  
>  	switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) {
>  	case DRM_EDID_DIGITAL_DEPTH_6:
> @@ -4489,7 +4514,9 @@ static void drm_add_display_info(struct drm_connector *connector,
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
> +	return quirks;
>  }
> +EXPORT_SYMBOL_GPL(drm_add_display_info);
>  
>  static int validate_displayid(u8 *displayid, int length, int idx)
>  {
> @@ -4645,8 +4672,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  		return 0;
>  	}
>  
> -	quirks = edid_get_quirks(edid);
> -
>  	drm_edid_to_eld(connector, edid);
>  
>  	/*
> @@ -4654,7 +4679,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	 * To avoid multiple parsing of same block, lets parse that map
>  	 * from sink info, before parsing CEA modes.
>  	 */
> -	drm_add_display_info(connector, edid, quirks);
> +	quirks = drm_add_display_info(connector, edid);
>  
>  	/*
>  	 * EDID spec says modes should be preferred in this order:
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b25d12ef120a..8d89a9c3748d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -465,6 +465,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);
>  struct edid *drm_edid_duplicate(const struct edid *edid);
> +void drm_reset_display_info(struct drm_connector *connector);
> +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid);
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
>  
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
> -- 
> 2.15.0.rc0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists