| Message ID | a5d38210e9eb5425f60ffa0243e1155f9ad689bf.1487241304.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(). > > FIXME: validate override edid, deduplicate firmware edid validation. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > 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
On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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? BR, Jani. > >> >> FIXME: validate override edid, deduplicate firmware edid validation. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> 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
On Thu, Feb 16, 2017 at 07:54:00PM +0200, Jani Nikula wrote: > On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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. I guess one way around this would be to skip the DDC check if the connector state has been forced. > > BR, > Jani. > > > > > >> > >> FIXME: validate override edid, deduplicate firmware edid validation. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> 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
On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Feb 16, 2017 at 07:54:00PM +0200, Jani Nikula wrote: >> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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. BR, Jani. > >> >> BR, >> Jani. >> >> >> > >> >> >> >> FIXME: validate override edid, deduplicate firmware edid validation. >> >> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> 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
On Thu, Feb 16, 2017 at 08:59:13PM +0200, Jani Nikula wrote: > On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Feb 16, 2017 at 07:54:00PM +0200, Jani Nikula wrote: > >> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@intel.com> > >> >> --- > >> >> 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
On Thu, Feb 16, 2017 at 09:14:45PM +0200, Ville Syrjälä wrote: > On Thu, Feb 16, 2017 at 08:59:13PM +0200, Jani Nikula wrote: > > On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Thu, Feb 16, 2017 at 07:54:00PM +0200, Jani Nikula wrote: > > >> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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. Iirc I just said (well, I hope that's what I said, didn't check with past self) that the current semantics we expose work like that. They might not be sensible. But anyway, I guess we need to tackle both the override edid and the connector status forcing in one patch approach, and I guess doing it here in drm_do_get_edid is indeed sensible. And if we can get away with it, deriving the force state from the override edid (if present), but not the other way round, would be good. Or at least the dvi-d vs. dvi-i part of connector forcing (which isn't something we use on i915, but which exists). -Daniel > > > > > BR, > > Jani. > > > > > > > >> > > >> BR, > > >> Jani. > > >> > > >> > > >> > > > >> >> > > >> >> FIXME: validate override edid, deduplicate firmware edid validation. > > >> >> > > >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > >> >> --- > > >> >> 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
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);
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. FIXME: validate override edid, deduplicate firmware edid validation. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++ drivers/gpu/drm/drm_probe_helper.c | 19 +------------------ 2 files changed, 16 insertions(+), 18 deletions(-)