Re: [Intel-gfx] [PATCH v7 17/22] drm/edid: add separate drm_edid_connector_add_modes()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 04, 2023 at 12:05:32PM +0200, Jani Nikula wrote:
> The original goal with drm_edid_connector_update() was to have a single
> call for updating the connector and adding probed modes, in this order,
> but that turned out to be problematic. Drivers that need to update the
> connector in the .detect() callback would end up updating the probed
> modes as well. Turns out the callback may be called so many times that
> the probed mode list fills up without bounds, and this is amplified by
> add_alternate_cea_modes() duplicating the CEA modes on every call,
> actually running out of memory on some machines.
> 
> Kudos to Imre Deak <imre.deak@intel.com> for explaining this to me.
> 
> Go back to having separate drm_edid_connector_update() and
> drm_edid_connector_add_modes() calls. The former may be called from
> .detect(), .force(), or .get_modes(), but the latter only from
> .get_modes().
> 
> Unlike drm_add_edid_modes(), have drm_edid_connector_add_modes() update
> the probed modes from the EDID property instead of the passed in
> EDID. This is mainly to enforce two things:
> 
> 1) drm_edid_connector_update() must be called before
>    drm_edid_connector_add_modes().
> 
>    Display info and quirks are needed for parsing the modes, and we
>    don't want to call update_display_info() again to ensure the info is
>    available, like drm_add_edid_modes() does.
> 
> 2) The same EDID is used for both updating the connector and adding the
>    probed modes.
> 
> Fortunately, the change is easy, because no driver has actually adopted
> drm_edid_connector_update(). Not even i915, and that's mainly because of
> the problem described above.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c         | 44 +++++++++++++++++++++++-------
>  drivers/gpu/drm/drm_probe_helper.c |  4 ++-
>  include/drm/drm_edid.h             |  2 ++
>  3 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 95c383220afc..a64c0807e97f 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6761,30 +6761,54 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>   * @connector: Connector
>   * @drm_edid: EDID
>   *
> - * Update the connector mode list, display info, ELD, HDR metadata, relevant
> - * properties, etc. from the passed in EDID.
> + * Update the connector display info, ELD, HDR metadata, relevant properties,
> + * etc. from the passed in EDID.
>   *
>   * If EDID is NULL, reset the information.
>   *
> - * Return: The number of modes added or 0 if we couldn't find any.
> + * Must be called before calling drm_edid_connector_add_modes().
> + *
> + * Return: 0 on success, negative error on errors.
>   */
>  int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *drm_edid)
>  {
> +	update_display_info(connector, drm_edid);
> +
> +	_drm_update_tile_info(connector, drm_edid);
> +
> +	return _drm_edid_connector_property_update(connector, drm_edid);
> +}
> +EXPORT_SYMBOL(drm_edid_connector_update);
> +
> +/**
> + * drm_edid_connector_add_modes - Update probed modes from the EDID property
> + * @connector: Connector
> + *
> + * Add the modes from the previously updated EDID property to the connector
> + * probed modes list.
> + *
> + * drm_edid_connector_update() must have been called before this to update the
> + * EDID property.
> + *
> + * Return: The number of modes added, or 0 if we couldn't find any.
> + */
> +int drm_edid_connector_add_modes(struct drm_connector *connector)
> +{
> +	const struct drm_edid *drm_edid = NULL;
>  	int count;
>  
> -	update_display_info(connector, drm_edid);
> +	if (connector->edid_blob_ptr)
> +		drm_edid = drm_edid_alloc(connector->edid_blob_ptr->data,
> +					  connector->edid_blob_ptr->length);
>  
>  	count = _drm_edid_connector_add_modes(connector, drm_edid);
>  
> -	_drm_update_tile_info(connector, drm_edid);
> -
> -	/* Note: Ignore errors for now. */
> -	_drm_edid_connector_property_update(connector, drm_edid);
> +	drm_edid_free(drm_edid);
>  
>  	return count;
>  }
> -EXPORT_SYMBOL(drm_edid_connector_update);
> +EXPORT_SYMBOL(drm_edid_connector_add_modes);
>  
>  static int _drm_connector_update_edid_property(struct drm_connector *connector,
>  					       const struct drm_edid *drm_edid)
> @@ -6839,7 +6863,7 @@ EXPORT_SYMBOL(drm_connector_update_edid_property);
>   * &drm_display_info structure and ELD in @connector with any information which
>   * can be derived from the edid.
>   *
> - * This function is deprecated. Use drm_edid_connector_update() instead.
> + * This function is deprecated. Use drm_edid_connector_add_modes() instead.
>   *
>   * Return: The number of modes added or 0 if we couldn't find any.
>   */
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1ea053cef557..26844befc6f5 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1139,7 +1139,9 @@ int drm_connector_helper_get_modes(struct drm_connector *connector)
>  	 * EDID. Otherwise, if the EDID is NULL, clear the connector
>  	 * information.
>  	 */
> -	count = drm_edid_connector_update(connector, drm_edid);
> +	drm_edid_connector_update(connector, drm_edid);
> +
> +	count = drm_edid_connector_add_modes(connector);
>  
>  	drm_edid_free(drm_edid);
>  
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 372963600f1d..70ae6c290bdc 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -609,6 +609,8 @@ const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
>  					    void *context);
>  int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *edid);
> +int drm_edid_connector_add_modes(struct drm_connector *connector);
> +
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel