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@xxxxxxxxx>
> > > > ---
> > > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
