Re: [PATCH v3 1/2] drm: handle override and firmware EDID at drm_do_get_edid() level

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

On Tue, Sep 12, 2017 at 11:19:26AM +0300, Jani Nikula wrote:
> Handle debugfs override edid and firmware edid at the low level to
> transparently and completely replace the real edid. Previously, we
> practically only used the modes from the override EDID, and none of the
> other data, such as audio parameters.
> 
> This change also prevents actual EDID reads when the EDID is to be
> overridden, but retains the DDC probe. This is useful if the reason for
> preferring override EDID are problems with reading the data, or
> corruption of the data.
> 
> Move firmware EDID loading from helper to core, as the functionality
> moves to lower level as well. This will result in a change of module
> parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> arguably makes more sense anyway.
> 
> Some future work remains related to override and firmware EDID
> validation. Like before, no validation is done for override EDID. The
> firmware EDID is validated separately in the loader. Some unification
> and deduplication would be in order, to validate all of them at the
> drm_do_get_edid() level, like "real" EDIDs.
> 
> v2: move firmware loading to core
> 
> v3: rebase, commit message refresh
> 
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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>
still holds

> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 +-
>  drivers/gpu/drm/Kconfig                         |  2 +-
>  drivers/gpu/drm/Makefile                        |  2 +-
>  drivers/gpu/drm/drm_edid.c                      | 15 +++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c              | 19 +------------------
>  5 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d9c171ce4190..9b393c29953f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -854,7 +854,7 @@
>  			The filter can be disabled or changed to another
>  			driver later using sysfs.
>  
> -	drm_kms_helper.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
> +	drm.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
>  			Broken monitors, graphic adapters, KVMs and EDIDless
>  			panels may send no or incorrect EDID data sets.
>  			This parameter allows to specify an EDID data sets
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index c5e1a8409285..c9f09fc298bb 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -110,7 +110,7 @@ config DRM_FBDEV_OVERALLOC
>  
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
> -	depends on DRM_KMS_HELPER
> +	depends on DRM
>  	help
>  	  Say Y here, if you want to use EDID data to be loaded from the
>  	  /lib/firmware directory or one of the provided built-in
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index df923119ac36..0ee184f56a60 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -28,6 +28,7 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> @@ -36,7 +37,6 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_scdc_helper.o drm_gem_framebuffer_helper.o
>  
>  drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6bb6337be920..00ddabfbf980 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1533,6 +1533,10 @@ static void connector_bad_edid(struct drm_connector *connector,
>   * level, drivers must make all reasonable efforts to expose it as an I2C
>   * adapter and use drm_get_edid() instead of abusing this function.
>   *
> + * The EDID may be overridden using debugfs override_edid or firmare EDID
> + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
> + * order. Having either of them bypasses actual EDID reads.
> + *
>   * Return: Pointer to valid EDID or NULL if we couldn't find any.
>   */
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
> @@ -1542,6 +1546,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  {
>  	int i, j = 0, valid_extensions = 0;
>  	u8 *edid, *new;
> +	struct edid *override = NULL;
> +
> +	if (connector->override_edid)
> +		override = drm_edid_duplicate((const struct edid *)
> +					      connector->edid_blob_ptr->data);
> +
> +	if (!override)
> +		override = drm_load_edid_firmware(connector);
> +
> +	if (!IS_ERR_OR_NULL(override))
> +		return override;
>  
>  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>  		return NULL;
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 904966cde32b..5840aabbf24e 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -353,8 +353,6 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
>   *    drm_mode_probed_add(). New modes start their life with status as OK.
>   *    Modes are added from a single source using the following priority order.
>   *
> - *    - debugfs 'override_edid' (used for testing only)
> - *    - firmware EDID (drm_load_edid_firmware())
>   *    - &drm_connector_helper_funcs.get_modes vfunc
>   *    - if the connector status is connector_status_connected, standard
>   *      VESA DMT modes up to 1024x768 are automatically added
> @@ -483,22 +481,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		goto prune;
>  	}
>  
> -	if (connector->override_edid) {
> -		struct edid *edid = (struct edid *) connector->edid_blob_ptr->data;
> -
> -		count = drm_add_edid_modes(connector, edid);
> -		drm_edid_to_eld(connector, edid);
> -	} else {
> -		struct edid *edid = drm_load_edid_firmware(connector);
> -		if (!IS_ERR_OR_NULL(edid)) {
> -			drm_mode_connector_update_edid_property(connector, edid);
> -			count = drm_add_edid_modes(connector, edid);
> -			drm_edid_to_eld(connector, edid);
> -			kfree(edid);
> -		}
> -		if (count == 0)
> -			count = (*connector_funcs->get_modes)(connector);
> -	}
> +	count = (*connector_funcs->get_modes)(connector);
>  
>  	if (count == 0 && connector->status == connector_status_connected)
>  		count = drm_add_modes_noedid(connector, 1024, 768);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx