On Thu, Feb 16, 2017 at 08:59:13PM +0200, Jani Nikula wrote:
> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Thu, Feb 16, 2017 at 07:54:00PM +0200, Jani Nikula wrote:
> >> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> >> > On Thu, Feb 16, 2017 at 12:36:45PM +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.
> >> >
> >> > Hmm. Isn't that a bad thing? If someone has broken DDC on their cable or
> >> > somethign then the override EDID wouldn't be returned by drm_get_edid().
> >>
> >> Isn't this in line with how this currently works?
> >
> > Not really. If connector->force is used and an override EDID is
> > provided, then drm_get_edid() likely never gets called, and the modes
> > come directly from the overide EDID. With your change forcing the
> > connector state wouldn't because we still wouldn't get any modes
> > from the override EDID due to the DDC check.
>
> Right.
>
> > I guess one way around this would be to skip the DDC check if the
> > connector state has been forced.
>
> I guess I'm asking if you think the whole approach here is
> workable. What we currently have is so flaky.
I'm 100% with you on that. So yes, I do like the idea.
I think the last time the override EDIDs were discussed someone was
suggesting that the EDID override itself should force the connector
state. IIRC Daniel didn't agree. I don't have a particularly strong
opinion either way, but I guess trying to preserve the current
semantics (ie. EDID override and connector status forcing are mostly
separate) would be the thing to aim for, at least initially.
>
> BR,
> Jani.
>
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> >>
> >> >> FIXME: validate override edid, deduplicate firmware edid validation.
> >> >>
> >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> >> ---
> >> >> drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++
> >> >> drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
> >> >> 2 files changed, 16 insertions(+), 18 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> >> index 362036360724..054e2d74eafc 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);
> >> >>
> >> >> if (count == 0 && connector->status == connector_status_connected)
> >> >> count = drm_add_modes_noedid(connector, 1024, 768);
> >> >> --
> >> >> 2.1.4
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
