| Message ID | 1427359320-9451-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 26, 2015 at 10:42:00AM +0200, Jani Nikula wrote: > If the user supplies EDID through firmware or debugfs override, the > driver callbacks are bypassed and the connector ELD does not get > updated, and audio fails. Set ELD for firmware and debugfs EDIDs too. > > There should be no harm in gratuitously doing this for non HDMI/DP > connectors, as it's still up to the driver to use the ELD, if any. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82349 > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=80691 > Reported-by: Emil <emilsvennesson@gmail.com> > Reported-by: Rob Engle <grenoble@gmail.com> > Tested-by: Jolan Luff <jolan@gormsby.com> > Cc: stable@vger.kernel.org > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Since it's harmless I wonder whether we shouldn't just do this in drm_add_edid_modes unconditionally. But this looks like the right minimal patch for -fixes, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > --- > drivers/gpu/drm/drm_edid_load.c | 1 + > drivers/gpu/drm/drm_probe_helper.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c > index 732cb6f8e653..4c0aa97aaf03 100644 > --- a/drivers/gpu/drm/drm_edid_load.c > +++ b/drivers/gpu/drm/drm_edid_load.c > @@ -287,6 +287,7 @@ int drm_load_edid_firmware(struct drm_connector *connector) > > drm_mode_connector_update_edid_property(connector, edid); > ret = drm_add_edid_modes(connector, edid); > + drm_edid_to_eld(connector, edid); > kfree(edid); > > return ret; > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 6591d48c1b9d..3fee587bc284 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -174,6 +174,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect > struct edid *edid = (struct edid *) connector->edid_blob_ptr->data; > > count = drm_add_edid_modes(connector, edid); > + drm_edid_to_eld(connector, edid); > } else > count = (*connector_funcs->get_modes)(connector); > } > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6058
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 303/303 303/303
SNB -2 304/304 302/304
IVB 339/339 339/339
BYT 287/287 287/287
HSW 362/362 362/362
BDW 310/310 310/310
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*SNB igt@pm_rpm@drm-resources-equal PASS(2) DMESG_FAIL(1)PASS(1)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*failed_to_enable_link_training@failed to enable link training
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_start_channel_equalization@failed to start channel equalization
*SNB igt@pm_rpm@modeset-non-lpsp-stress-no-wait PASS(2) DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
Note: You need to pay more attention to line start with '*'
On Thu, 26 Mar 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Mar 26, 2015 at 10:42:00AM +0200, Jani Nikula wrote: >> If the user supplies EDID through firmware or debugfs override, the >> driver callbacks are bypassed and the connector ELD does not get >> updated, and audio fails. Set ELD for firmware and debugfs EDIDs too. >> >> There should be no harm in gratuitously doing this for non HDMI/DP >> connectors, as it's still up to the driver to use the ELD, if any. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82349 >> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=80691 >> Reported-by: Emil <emilsvennesson@gmail.com> >> Reported-by: Rob Engle <grenoble@gmail.com> >> Tested-by: Jolan Luff <jolan@gormsby.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Since it's harmless I wonder whether we shouldn't just do this in > drm_add_edid_modes unconditionally. But this looks like the right minimal > patch for -fixes, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> While I was hoping to gather review from outside of drm/i915 folks, I picked this up and pushed to our new topic/drm-fixes branch of [1]. I intend to keep picking up (occasional, non-controversial) drm core fixes aimed at the current development (-rc) kernels, to ensure they're not dropped, and sending pull requests to Dave as needed. He'll have the final call whether to pull or not, of course. This is similar to what Daniel does with the topic/drm-misc branch for drm-next. Please let me know if you have any feedback on this. Thanks, Jani. [1] http://cgit.freedesktop.org/drm-intel
On Fri, Mar 27, 2015 at 8:08 AM, Jani Nikula <jani.nikula@intel.com> wrote: > On Thu, 26 Mar 2015, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, Mar 26, 2015 at 10:42:00AM +0200, Jani Nikula wrote: >>> If the user supplies EDID through firmware or debugfs override, the >>> driver callbacks are bypassed and the connector ELD does not get >>> updated, and audio fails. Set ELD for firmware and debugfs EDIDs too. >>> >>> There should be no harm in gratuitously doing this for non HDMI/DP >>> connectors, as it's still up to the driver to use the ELD, if any. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82349 >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=80691 >>> Reported-by: Emil <emilsvennesson@gmail.com> >>> Reported-by: Rob Engle <grenoble@gmail.com> >>> Tested-by: Jolan Luff <jolan@gormsby.com> >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> Since it's harmless I wonder whether we shouldn't just do this in >> drm_add_edid_modes unconditionally. But this looks like the right minimal >> patch for -fixes, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > While I was hoping to gather review from outside of drm/i915 folks, I > picked this up and pushed to our new topic/drm-fixes branch of [1]. > > I intend to keep picking up (occasional, non-controversial) drm core > fixes aimed at the current development (-rc) kernels, to ensure they're > not dropped, and sending pull requests to Dave as needed. He'll have the > final call whether to pull or not, of course. This is similar to what > Daniel does with the topic/drm-misc branch for drm-next. > > Please let me know if you have any feedback on this. The patch seems fine to me. However, if we are always going to set the ELD for the override cases, why don't we also always set it for the non-override cases rather than making each driver do it. Alex > > Thanks, > Jani. > > > [1] http://cgit.freedesktop.org/drm-intel > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, 27 Mar 2015, Alex Deucher <alexdeucher@gmail.com> wrote: > On Fri, Mar 27, 2015 at 8:08 AM, Jani Nikula <jani.nikula@intel.com> wrote: >> On Thu, 26 Mar 2015, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, Mar 26, 2015 at 10:42:00AM +0200, Jani Nikula wrote: >>>> If the user supplies EDID through firmware or debugfs override, the >>>> driver callbacks are bypassed and the connector ELD does not get >>>> updated, and audio fails. Set ELD for firmware and debugfs EDIDs too. >>>> >>>> There should be no harm in gratuitously doing this for non HDMI/DP >>>> connectors, as it's still up to the driver to use the ELD, if any. >>>> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82349 >>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=80691 >>>> Reported-by: Emil <emilsvennesson@gmail.com> >>>> Reported-by: Rob Engle <grenoble@gmail.com> >>>> Tested-by: Jolan Luff <jolan@gormsby.com> >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> >>> Since it's harmless I wonder whether we shouldn't just do this in >>> drm_add_edid_modes unconditionally. But this looks like the right minimal >>> patch for -fixes, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> While I was hoping to gather review from outside of drm/i915 folks, I >> picked this up and pushed to our new topic/drm-fixes branch of [1]. >> >> I intend to keep picking up (occasional, non-controversial) drm core >> fixes aimed at the current development (-rc) kernels, to ensure they're >> not dropped, and sending pull requests to Dave as needed. He'll have the >> final call whether to pull or not, of course. This is similar to what >> Daniel does with the topic/drm-misc branch for drm-next. >> >> Please let me know if you have any feedback on this. > > > The patch seems fine to me. However, if we are always going to set > the ELD for the override cases, why don't we also always set it for > the non-override cases rather than making each driver do it. So I think this is a good minimal patch for fixes/stable as a first step. But I agree, we should think about the follow-up. I already had a glance before, and doing so really begs the question why we wouldn't add a helper to handle all of drm_add_edid_modes, drm_edid_to_eld, and drm_mode_connector_update_edid_property properly? Similar to intel_connector_update_modes in i915/intel_modes.c. All drivers do this stuff, although with subtle differences especially wrt error handling. BR, Jani. > > Alex > >> >> Thanks, >> Jani. >> >> >> [1] http://cgit.freedesktop.org/drm-intel >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Fri, Mar 27, 2015 at 12:02 PM, Jani Nikula <jani.nikula@intel.com> wrote: > On Fri, 27 Mar 2015, Alex Deucher <alexdeucher@gmail.com> wrote: >> On Fri, Mar 27, 2015 at 8:08 AM, Jani Nikula <jani.nikula@intel.com> wrote: >>> On Thu, 26 Mar 2015, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Thu, Mar 26, 2015 at 10:42:00AM +0200, Jani Nikula wrote: >>>>> If the user supplies EDID through firmware or debugfs override, the >>>>> driver callbacks are bypassed and the connector ELD does not get >>>>> updated, and audio fails. Set ELD for firmware and debugfs EDIDs too. >>>>> >>>>> There should be no harm in gratuitously doing this for non HDMI/DP >>>>> connectors, as it's still up to the driver to use the ELD, if any. >>>>> >>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82349 >>>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=80691 >>>>> Reported-by: Emil <emilsvennesson@gmail.com> >>>>> Reported-by: Rob Engle <grenoble@gmail.com> >>>>> Tested-by: Jolan Luff <jolan@gormsby.com> >>>>> Cc: stable@vger.kernel.org >>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>>> >>>> Since it's harmless I wonder whether we shouldn't just do this in >>>> drm_add_edid_modes unconditionally. But this looks like the right minimal >>>> patch for -fixes, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> >>> While I was hoping to gather review from outside of drm/i915 folks, I >>> picked this up and pushed to our new topic/drm-fixes branch of [1]. >>> >>> I intend to keep picking up (occasional, non-controversial) drm core >>> fixes aimed at the current development (-rc) kernels, to ensure they're >>> not dropped, and sending pull requests to Dave as needed. He'll have the >>> final call whether to pull or not, of course. This is similar to what >>> Daniel does with the topic/drm-misc branch for drm-next. >>> >>> Please let me know if you have any feedback on this. >> >> >> The patch seems fine to me. However, if we are always going to set >> the ELD for the override cases, why don't we also always set it for >> the non-override cases rather than making each driver do it. > > So I think this is a good minimal patch for fixes/stable as a first > step. > > But I agree, we should think about the follow-up. I already had a glance > before, and doing so really begs the question why we wouldn't add a > helper to handle all of drm_add_edid_modes, drm_edid_to_eld, and > drm_mode_connector_update_edid_property properly? Similar to > intel_connector_update_modes in i915/intel_modes.c. All drivers do this > stuff, although with subtle differences especially wrt error handling. > Agreed. Alex > BR, > Jani. > > >> >> Alex >> >>> >>> Thanks, >>> Jani. >>> >>> >>> [1] http://cgit.freedesktop.org/drm-intel >>> >>> -- >>> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 732cb6f8e653..4c0aa97aaf03 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -287,6 +287,7 @@ int drm_load_edid_firmware(struct drm_connector *connector) drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); + drm_edid_to_eld(connector, edid); kfree(edid); return ret; diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 6591d48c1b9d..3fee587bc284 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -174,6 +174,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect struct edid *edid = (struct edid *) connector->edid_blob_ptr->data; count = drm_add_edid_modes(connector, edid); + drm_edid_to_eld(connector, edid); } else count = (*connector_funcs->get_modes)(connector); }