Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

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

On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> On Fri, Feb 17, 2017 at 05:20:54PM +0200, 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. This also prevents actual EDID reads when the EDID is to be
> > overridden, but retains the DDC probe.
> > 
> > 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.
> > 
> > FIXME: validate override edid, deduplicate firmware edid validation.
> > 
> > v2: move firmware loading to core
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  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 +------------------
> >  4 files changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 90bc65d07a35..f983ef60299c 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> >  
> >  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 92de3991fa56..a10ac095608f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -27,13 +27,13 @@ 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 \
> >  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> >  		drm_simple_kms_helper.o drm_modeset_helper.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 e1743ab276dc..4007998d5ce3 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1309,6 +1309,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()), 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,
> > @@ -1318,6 +1322,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 358957118ca9..871326cbc465 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
> >   *    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
> > @@ -305,22 +303,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);
> 
> What happens for drivers which cache the edid in their probe function, and
> in their get_modes function only parse the already retrived edid?

Hmm. Good question. I guess currently they'll just keep using that
initial EDID (which may or may not come from the firmware EDID loader).
but overriding at runtime likely won't work.

For i915 that only happens for eDP/LVDS/DSI and such. And for those we
also parse the panel fixed mode from the EDID at probe time. So there
isn't much you could gain from runtime override unless we change the
entire implementation to not hang on to the fixed mode either.

> This
> becomes fun in combination with the output forcing, where the connector
> status forcing prevents ->detect from being run ...

We do have the ->force() hook for those cases.

> I still think we can't handle override edid without also taking connector
> forcing into account.

After these patches the two are somewhat linked. The force status
will affect whether drm_get_edid() will even return anything (doesn't
matter if there's an override EDID or not), but the presence of the
override EDID won't force the connector status. To me that seems
like a perfectly sensible approach.

> -Daniel
> 
> >  
> >  	if (count == 0 && connector->status == connector_status_connected)
> >  		count = drm_add_modes_noedid(connector, 1024, 768);
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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