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 Mon, Feb 27, 2017 at 05:19:21PM +0100, Daniel Vetter wrote:
> On Mon, Feb 27, 2017 at 05:09:44PM +0200, Ville Syrjälä wrote:
> > 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.
> 
> Yes, that's the problem I'm seeing. Atm our igts both force the connector
> state and override the edid, which means they'd be defunct. At least I
> think they'd fail ...

They should work just fine.

> 
> > > 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.
> 
> How does the EDID override affect the status?

It doesn't.

> If there's an issue with hpd
> on e.g. DP, it still won't. I'm not even sure this would affect all the
> places where we realy on ddc detection.

I think the lack of DPCD read in ->force() is the only real problem
I see in the DP code.

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