* [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP.
@ 2020-02-11 11:40 Stanislav Lisovskiy
2020-02-11 12:16 ` Imre Deak
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Stanislav Lisovskiy @ 2020-02-11 11:40 UTC (permalink / raw)
To: intel-gfx
I guess it would still be nice to make the code less confusing
and do not call eDP specific function, for non-eDP connectors
just to immediately return true(success) value as a hack.
So simply extracted that check out from this function,
that we simply don't call it for non-eDP connectors. Bingo.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index f4dede6253f8..9bd36197a43d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
intel_wakeref_t wakeref;
struct edid *edid;
- if (!intel_dp_is_edp(intel_dp))
- return true;
-
INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work);
/*
@@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
intel_dp_mst_encoder_init(intel_dig_port,
intel_connector->base.base.id);
- if (!intel_edp_init_connector(intel_dp, intel_connector)) {
- intel_dp_aux_fini(intel_dp);
- intel_dp_mst_encoder_cleanup(intel_dig_port);
- goto fail;
+ if (intel_dp_is_edp(intel_dp)) {
+ if (!intel_edp_init_connector(intel_dp, intel_connector)) {
+ intel_dp_aux_fini(intel_dp);
+ intel_dp_mst_encoder_cleanup(intel_dig_port);
+ goto fail;
+ }
}
intel_dp_add_properties(intel_dp, connector);
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 11:40 [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP Stanislav Lisovskiy @ 2020-02-11 12:16 ` Imre Deak 2020-02-11 13:03 ` Jani Nikula 2020-02-13 15:11 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2 siblings, 0 replies; 10+ messages in thread From: Imre Deak @ 2020-02-11 12:16 UTC (permalink / raw) To: Stanislav Lisovskiy; +Cc: intel-gfx On Tue, Feb 11, 2020 at 01:40:38PM +0200, Stanislav Lisovskiy wrote: > I guess it would still be nice to make the code less confusing > and do not call eDP specific function, for non-eDP connectors > just to immediately return true(success) value as a hack. > > So simply extracted that check out from this function, > that we simply don't call it for non-eDP connectors. Bingo. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f4dede6253f8..9bd36197a43d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > intel_wakeref_t wakeref; > struct edid *edid; > > - if (!intel_dp_is_edp(intel_dp)) > - return true; > - > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work); > > /* > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > - intel_dp_aux_fini(intel_dp); > - intel_dp_mst_encoder_cleanup(intel_dig_port); > - goto fail; > + if (intel_dp_is_edp(intel_dp)) { > + if (!intel_edp_init_connector(intel_dp, intel_connector)) { > + intel_dp_aux_fini(intel_dp); > + intel_dp_mst_encoder_cleanup(intel_dig_port); > + goto fail; > + } Yes, one jump less when reading the code. You could also use a&&b to reduce the indent: Reviewed-by: Imre Deak <imre.deak@intel.com> > } > > intel_dp_add_properties(intel_dp, connector); > -- > 2.24.1.485.gad05a3d8e5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 11:40 [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP Stanislav Lisovskiy 2020-02-11 12:16 ` Imre Deak @ 2020-02-11 13:03 ` Jani Nikula 2020-02-11 13:33 ` Lisovskiy, Stanislav 2020-02-13 15:11 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2 siblings, 1 reply; 10+ messages in thread From: Jani Nikula @ 2020-02-11 13:03 UTC (permalink / raw) To: Stanislav Lisovskiy, intel-gfx On Tue, 11 Feb 2020, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > I guess it would still be nice to make the code less confusing > and do not call eDP specific function, for non-eDP connectors > just to immediately return true(success) value as a hack. > > So simply extracted that check out from this function, > that we simply don't call it for non-eDP connectors. Bingo. Fair enough, I guess... Though I could be persuaded to take a patch for the reverse, because generally we prefer localized knowledge in the callee than in a potentially irrelevant place in the caller. Consider the intel_dp_mst_encoder_init() call in the context of this patch. We call it, and the function itself decides whether MST init is relevant or not. But I presume you wouldn't suggest pulling in all the conditions from there one level higher? Would your opinion on intel_edp_init_connector() be different if the conditions were more than just the single intel_dp_is_edp()? Or if we moved all of eDP support to a separate file? BR, Jani. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f4dede6253f8..9bd36197a43d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > intel_wakeref_t wakeref; > struct edid *edid; > > - if (!intel_dp_is_edp(intel_dp)) > - return true; > - > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work); > > /* > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > - intel_dp_aux_fini(intel_dp); > - intel_dp_mst_encoder_cleanup(intel_dig_port); > - goto fail; > + if (intel_dp_is_edp(intel_dp)) { > + if (!intel_edp_init_connector(intel_dp, intel_connector)) { > + intel_dp_aux_fini(intel_dp); > + intel_dp_mst_encoder_cleanup(intel_dig_port); > + goto fail; > + } > } > > intel_dp_add_properties(intel_dp, connector); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 13:03 ` Jani Nikula @ 2020-02-11 13:33 ` Lisovskiy, Stanislav 2020-02-11 13:55 ` Jani Nikula 0 siblings, 1 reply; 10+ messages in thread From: Lisovskiy, Stanislav @ 2020-02-11 13:33 UTC (permalink / raw) To: intel-gfx, jani.nikula On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, Stanislav Lisovskiy < > stanislav.lisovskiy@intel.com> wrote: > > I guess it would still be nice to make the code less confusing > > and do not call eDP specific function, for non-eDP connectors > > just to immediately return true(success) value as a hack. > > > > So simply extracted that check out from this function, > > that we simply don't call it for non-eDP connectors. Bingo. > > Fair enough, I guess... > > Though I could be persuaded to take a patch for the reverse, because > generally we prefer localized knowledge in the callee than in a > potentially irrelevant place in the caller. > > Consider the intel_dp_mst_encoder_init() call in the context of this > patch. We call it, and the function itself decides whether MST init > is > relevant or not. But I presume you wouldn't suggest pulling in all > the > conditions from there one level higher? > > Would your opinion on intel_edp_init_connector() be different if the > conditions were more than just the single intel_dp_is_edp()? Or if we > moved all of eDP support to a separate file? Well, to me at least intel_edp_init_connector means that we are going to init an eDP connector, which already assumes that, we already should know that this is an eDP connector :) Otherwise it should have somewhat different name, not saying that this is the best philosophy, but generally I would prefer the functions to be solely responsible for a single thing so that that init function is supposed to do only init, but not also some detection/checking or any other side effects. So that there is a clear visibility what we are doing, if it's an init then we really do only init or if we supposed to detect something first, let it be a separate thing.. Also this uses a return value in confusing way, i.e returning "Success" status for non-eDP from intel_edp_init_connector seems kind of confusing. Stan > > BR, > Jani. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index f4dede6253f8..9bd36197a43d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct > > intel_dp *intel_dp, > > intel_wakeref_t wakeref; > > struct edid *edid; > > > > - if (!intel_dp_is_edp(intel_dp)) > > - return true; > > - > > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > > edp_panel_vdd_work); > > > > /* > > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > > intel_digital_port *intel_dig_port, > > intel_dp_mst_encoder_init(intel_dig_port, > > intel_connector->base.base.id); > > > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > > - intel_dp_aux_fini(intel_dp); > > - intel_dp_mst_encoder_cleanup(intel_dig_port); > > - goto fail; > > + if (intel_dp_is_edp(intel_dp)) { > > + if (!intel_edp_init_connector(intel_dp, > > intel_connector)) { > > + intel_dp_aux_fini(intel_dp); > > + intel_dp_mst_encoder_cleanup(intel_dig_port); > > + goto fail; > > + } > > } > > > > intel_dp_add_properties(intel_dp, connector); > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 13:33 ` Lisovskiy, Stanislav @ 2020-02-11 13:55 ` Jani Nikula 2020-02-11 14:03 ` Lisovskiy, Stanislav 2020-02-11 14:21 ` Imre Deak 0 siblings, 2 replies; 10+ messages in thread From: Jani Nikula @ 2020-02-11 13:55 UTC (permalink / raw) To: Lisovskiy, Stanislav, intel-gfx On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: >> On Tue, 11 Feb 2020, Stanislav Lisovskiy < >> stanislav.lisovskiy@intel.com> wrote: >> > I guess it would still be nice to make the code less confusing >> > and do not call eDP specific function, for non-eDP connectors >> > just to immediately return true(success) value as a hack. >> > >> > So simply extracted that check out from this function, >> > that we simply don't call it for non-eDP connectors. Bingo. >> >> Fair enough, I guess... >> >> Though I could be persuaded to take a patch for the reverse, because >> generally we prefer localized knowledge in the callee than in a >> potentially irrelevant place in the caller. >> >> Consider the intel_dp_mst_encoder_init() call in the context of this >> patch. We call it, and the function itself decides whether MST init >> is >> relevant or not. But I presume you wouldn't suggest pulling in all >> the >> conditions from there one level higher? >> >> Would your opinion on intel_edp_init_connector() be different if the >> conditions were more than just the single intel_dp_is_edp()? Or if we >> moved all of eDP support to a separate file? > > Well, to me at least intel_edp_init_connector means that we are going > to init an eDP connector, which already assumes that, we already should > know that this is an eDP connector :) > Otherwise it should have somewhat different name, not saying that this > is the best philosophy, but generally I would prefer the functions to > be solely responsible for a single thing so that that init function is > supposed to do only init, but not also some detection/checking or any > other side effects. > > So that there is a clear visibility what we are doing, if it's an init > then we really do only init or if we supposed to detect something > first, let it be a separate thing.. > > Also this uses a return value in confusing way, i.e returning "Success" > status for non-eDP from intel_edp_init_connector seems kind of > confusing. Again, how is this different from intel_dp_mst_encoder_init()? With the *exactly* same rationale you'd end up with this: if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && !(INTEL_GEN(i915) < 12 && port == PORT_A) && !(INTEL_GEN(i915) < 11 && port == PORT_E)) intel_dp_mst_encoder_init(intel_dig_port, intel_connector->base.base.id); Surely the information is better localized in a SPOT in the MST code? BR, Jani. > > Stan > >> >> BR, >> Jani. >> >> >> > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- >> > 1 file changed, 6 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >> > b/drivers/gpu/drm/i915/display/intel_dp.c >> > index f4dede6253f8..9bd36197a43d 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct >> > intel_dp *intel_dp, >> > intel_wakeref_t wakeref; >> > struct edid *edid; >> > >> > - if (!intel_dp_is_edp(intel_dp)) >> > - return true; >> > - >> > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, >> > edp_panel_vdd_work); >> > >> > /* >> > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct >> > intel_digital_port *intel_dig_port, >> > intel_dp_mst_encoder_init(intel_dig_port, >> > intel_connector->base.base.id); >> > >> > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { >> > - intel_dp_aux_fini(intel_dp); >> > - intel_dp_mst_encoder_cleanup(intel_dig_port); >> > - goto fail; >> > + if (intel_dp_is_edp(intel_dp)) { >> > + if (!intel_edp_init_connector(intel_dp, >> > intel_connector)) { >> > + intel_dp_aux_fini(intel_dp); >> > + intel_dp_mst_encoder_cleanup(intel_dig_port); >> > + goto fail; >> > + } >> > } >> > >> > intel_dp_add_properties(intel_dp, connector); >> >> -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 13:55 ` Jani Nikula @ 2020-02-11 14:03 ` Lisovskiy, Stanislav 2020-02-11 14:12 ` Jani Nikula 2020-02-11 14:21 ` Imre Deak 1 sibling, 1 reply; 10+ messages in thread From: Lisovskiy, Stanislav @ 2020-02-11 14:03 UTC (permalink / raw) To: intel-gfx, jani.nikula On Tue, 2020-02-11 at 15:55 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" < > stanislav.lisovskiy@intel.com> wrote: > > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > > > On Tue, 11 Feb 2020, Stanislav Lisovskiy < > > > stanislav.lisovskiy@intel.com> wrote: > > > > I guess it would still be nice to make the code less confusing > > > > and do not call eDP specific function, for non-eDP connectors > > > > just to immediately return true(success) value as a hack. > > > > > > > > So simply extracted that check out from this function, > > > > that we simply don't call it for non-eDP connectors. Bingo. > > > > > > Fair enough, I guess... > > > > > > Though I could be persuaded to take a patch for the reverse, > > > because > > > generally we prefer localized knowledge in the callee than in a > > > potentially irrelevant place in the caller. > > > > > > Consider the intel_dp_mst_encoder_init() call in the context of > > > this > > > patch. We call it, and the function itself decides whether MST > > > init > > > is > > > relevant or not. But I presume you wouldn't suggest pulling in > > > all > > > the > > > conditions from there one level higher? > > > > > > Would your opinion on intel_edp_init_connector() be different if > > > the > > > conditions were more than just the single intel_dp_is_edp()? Or > > > if we > > > moved all of eDP support to a separate file? > > > > Well, to me at least intel_edp_init_connector means that we are > > going > > to init an eDP connector, which already assumes that, we already > > should > > know that this is an eDP connector :) > > Otherwise it should have somewhat different name, not saying that > > this > > is the best philosophy, but generally I would prefer the functions > > to > > be solely responsible for a single thing so that that init function > > is > > supposed to do only init, but not also some detection/checking or > > any > > other side effects. > > > > So that there is a clear visibility what we are doing, if it's an > > init > > then we really do only init or if we supposed to detect something > > first, let it be a separate thing.. > > > > Also this uses a return value in confusing way, i.e returning > > "Success" > > status for non-eDP from intel_edp_init_connector seems kind of > > confusing. > > Again, how is this different from intel_dp_mst_encoder_init()? > > With the *exactly* same rationale you'd end up with this: > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector- > >base.base.id); > > Surely the information is better localized in a SPOT in the MST code? Well, you can just take all those checks and put them into separate function. Something like: bool intel_dp_supports_mst(intel_dp) { if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && !(INTEL_GEN(i915) < 12 && port == PORT_A) && !(INTEL_GEN(i915) < 11 && port == PORT_E)) return true; return false; } so, then you would have it nicely looking and understandable: if (intel_dp_supports_mst(intel_dp)) intel_dp_mst_encoder_init(intel_dig_port, intel_connector->base.base.id); Anyway, I'm _not_ stating that this is _always_ the best way, but I don't see at least any reasons currently why it couldn't be done so. Stan > > > BR, > Jani. > > > > > Stan > > > > > > > > BR, > > > Jani. > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy < > > > > stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index f4dede6253f8..9bd36197a43d 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -7370,9 +7370,6 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > intel_wakeref_t wakeref; > > > > struct edid *edid; > > > > > > > > - if (!intel_dp_is_edp(intel_dp)) > > > > - return true; > > > > - > > > > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > > > > edp_panel_vdd_work); > > > > > > > > /* > > > > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > > > > intel_digital_port *intel_dig_port, > > > > intel_dp_mst_encoder_init(intel_dig_port, > > > > intel_connector- > > > > >base.base.id); > > > > > > > > - if (!intel_edp_init_connector(intel_dp, > > > > intel_connector)) { > > > > - intel_dp_aux_fini(intel_dp); > > > > - intel_dp_mst_encoder_cleanup(intel_dig_port); > > > > - goto fail; > > > > + if (intel_dp_is_edp(intel_dp)) { > > > > + if (!intel_edp_init_connector(intel_dp, > > > > intel_connector)) { > > > > + intel_dp_aux_fini(intel_dp); > > > > + intel_dp_mst_encoder_cleanup(intel_dig_ > > > > port); > > > > + goto fail; > > > > + } > > > > } > > > > > > > > intel_dp_add_properties(intel_dp, connector); > > > > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 14:03 ` Lisovskiy, Stanislav @ 2020-02-11 14:12 ` Jani Nikula 2020-02-11 14:27 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 10+ messages in thread From: Jani Nikula @ 2020-02-11 14:12 UTC (permalink / raw) To: Lisovskiy, Stanislav, intel-gfx On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > Well, you can just take all those checks and put them into separate > function. Something like: > > bool intel_dp_supports_mst(intel_dp) { > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > return true; > return false; > } > > so, then you would have it nicely looking and understandable: > > if (intel_dp_supports_mst(intel_dp)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > Anyway, I'm _not_ stating that this is _always_ the best way, but > I don't see at least any reasons currently why it couldn't be done so. It's fine, until you realize you need to call a function with the condition from more than one place, and you need to remember to have the same conditions in all the places. So the condition is no longer in one isolated place. It's not like we haven't thought about this before. ;) BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 14:12 ` Jani Nikula @ 2020-02-11 14:27 ` Lisovskiy, Stanislav 0 siblings, 0 replies; 10+ messages in thread From: Lisovskiy, Stanislav @ 2020-02-11 14:27 UTC (permalink / raw) To: intel-gfx, jani.nikula On Tue, 2020-02-11 at 16:12 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" < > stanislav.lisovskiy@intel.com> wrote: > > Well, you can just take all those checks and put them into separate > > function. Something like: > > > > bool intel_dp_supports_mst(intel_dp) { > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > > return true; > > return false; > > } > > > > so, then you would have it nicely looking and understandable: > > > > if (intel_dp_supports_mst(intel_dp)) > > intel_dp_mst_encoder_init(intel_dig_port, > > intel_connector->base.base.id); > > > > Anyway, I'm _not_ stating that this is _always_ the best way, but > > I don't see at least any reasons currently why it couldn't be done > > so. > > It's fine, until you realize you need to call a function with the > condition from more than one place, and you need to remember to have > the > same conditions in all the places. So the condition is no longer in > one > isolated place. It's not like we haven't thought about this before. > ;) Well, that is why I didn't say that this is always a best approach. :) Sure, if I had to do lots of calls of this function(even though this should be reviewed then, why we have to do it in a multiple places), I would then just put this check and init together into some helper function. Stan > > BR, > Jani. > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 13:55 ` Jani Nikula 2020-02-11 14:03 ` Lisovskiy, Stanislav @ 2020-02-11 14:21 ` Imre Deak 1 sibling, 0 replies; 10+ messages in thread From: Imre Deak @ 2020-02-11 14:21 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Tue, Feb 11, 2020 at 03:55:45PM +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > >> On Tue, 11 Feb 2020, Stanislav Lisovskiy < > >> stanislav.lisovskiy@intel.com> wrote: > >> > I guess it would still be nice to make the code less confusing > >> > and do not call eDP specific function, for non-eDP connectors > >> > just to immediately return true(success) value as a hack. > >> > > >> > So simply extracted that check out from this function, > >> > that we simply don't call it for non-eDP connectors. Bingo. > >> > >> Fair enough, I guess... > >> > >> Though I could be persuaded to take a patch for the reverse, because > >> generally we prefer localized knowledge in the callee than in a > >> potentially irrelevant place in the caller. > >> > >> Consider the intel_dp_mst_encoder_init() call in the context of this > >> patch. We call it, and the function itself decides whether MST init > >> is > >> relevant or not. But I presume you wouldn't suggest pulling in all > >> the > >> conditions from there one level higher? > >> > >> Would your opinion on intel_edp_init_connector() be different if the > >> conditions were more than just the single intel_dp_is_edp()? Or if we > >> moved all of eDP support to a separate file? > > > > Well, to me at least intel_edp_init_connector means that we are going > > to init an eDP connector, which already assumes that, we already should > > know that this is an eDP connector :) > > Otherwise it should have somewhat different name, not saying that this > > is the best philosophy, but generally I would prefer the functions to > > be solely responsible for a single thing so that that init function is > > supposed to do only init, but not also some detection/checking or any > > other side effects. > > > > So that there is a clear visibility what we are doing, if it's an init > > then we really do only init or if we supposed to detect something > > first, let it be a separate thing.. > > > > Also this uses a return value in confusing way, i.e returning "Success" > > status for non-eDP from intel_edp_init_connector seems kind of > > confusing. > > Again, how is this different from intel_dp_mst_encoder_init()? > > With the *exactly* same rationale you'd end up with this: > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > Surely the information is better localized in a SPOT in the MST code? I find it also clearer to bring the intel_dp_is_edp() check out from intel_edp_init_connector(): we init the connector to be either an eDP or DP connector. Atm it's not clear after intel_edp_init_connector() returns success if the connector is eDP or DP type at that point. Stan's change would improve that. > > > BR, > Jani. > > > > > Stan > > > >> > >> BR, > >> Jani. > >> > >> > >> > > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > >> > 1 file changed, 6 insertions(+), 7 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> > b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index f4dede6253f8..9bd36197a43d 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct > >> > intel_dp *intel_dp, > >> > intel_wakeref_t wakeref; > >> > struct edid *edid; > >> > > >> > - if (!intel_dp_is_edp(intel_dp)) > >> > - return true; > >> > - > >> > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > >> > edp_panel_vdd_work); > >> > > >> > /* > >> > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > >> > intel_digital_port *intel_dig_port, > >> > intel_dp_mst_encoder_init(intel_dig_port, > >> > intel_connector->base.base.id); > >> > > >> > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > >> > - intel_dp_aux_fini(intel_dp); > >> > - intel_dp_mst_encoder_cleanup(intel_dig_port); > >> > - goto fail; > >> > + if (intel_dp_is_edp(intel_dp)) { > >> > + if (!intel_edp_init_connector(intel_dp, > >> > intel_connector)) { > >> > + intel_dp_aux_fini(intel_dp); > >> > + intel_dp_mst_encoder_cleanup(intel_dig_port); > >> > + goto fail; > >> > + } > >> > } > >> > > >> > intel_dp_add_properties(intel_dp, connector); > >> > >> > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Call intel_edp_init_connector only for eDP. 2020-02-11 11:40 [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP Stanislav Lisovskiy 2020-02-11 12:16 ` Imre Deak 2020-02-11 13:03 ` Jani Nikula @ 2020-02-13 15:11 ` Patchwork 2 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2020-02-13 15:11 UTC (permalink / raw) To: Stanislav Lisovskiy; +Cc: intel-gfx == Series Details == Series: drm/i915: Call intel_edp_init_connector only for eDP. URL : https://patchwork.freedesktop.org/series/73290/ State : success == Summary == CI Bug Log - changes from CI_DRM_7916_full -> Patchwork_16518_full ==================================================== Summary ------- **SUCCESS** No regressions found. Known issues ------------ Here are the changes found in Patchwork_16518_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_busy@extended-parallel-vcs1: - shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#112080]) +5 similar issues [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb2/igt@gem_busy@extended-parallel-vcs1.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb7/igt@gem_busy@extended-parallel-vcs1.html * igt@gem_exec_schedule@deep-bsd: - shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#112146]) +5 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb6/igt@gem_exec_schedule@deep-bsd.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb1/igt@gem_exec_schedule@deep-bsd.html * igt@i915_selftest@live_blt: - shard-snb: [PASS][5] -> [INCOMPLETE][6] ([i915#82]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb2/igt@i915_selftest@live_blt.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb6/igt@i915_selftest@live_blt.html * igt@kms_color@pipe-c-ctm-max: - shard-skl: [PASS][7] -> [FAIL][8] ([i915#168]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl2/igt@kms_color@pipe-c-ctm-max.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl5/igt@kms_color@pipe-c-ctm-max.html * igt@kms_flip@2x-plain-flip-fb-recreate-interruptible: - shard-glk: [PASS][9] -> [FAIL][10] ([i915#34]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-glk3/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-glk7/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible.html * igt@kms_flip@flip-vs-expired-vblank: - shard-skl: [PASS][11] -> [FAIL][12] ([i915#79]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl1/igt@kms_flip@flip-vs-expired-vblank.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl9/igt@kms_flip@flip-vs-expired-vblank.html * igt@kms_flip@flip-vs-suspend-interruptible: - shard-kbl: [PASS][13] -> [DMESG-WARN][14] ([i915#180]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-kbl3/igt@kms_flip@flip-vs-suspend-interruptible.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-kbl7/igt@kms_flip@flip-vs-suspend-interruptible.html * igt@kms_frontbuffer_tracking@fbc-suspend: - shard-apl: [PASS][15] -> [DMESG-WARN][16] ([i915#180]) +6 similar issues [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-apl6/igt@kms_frontbuffer_tracking@fbc-suspend.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-apl6/igt@kms_frontbuffer_tracking@fbc-suspend.html * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt: - shard-tglb: [PASS][17] -> [SKIP][18] ([i915#668]) +3 similar issues [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt.html * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc: - shard-skl: [PASS][19] -> [FAIL][20] ([fdo#108145] / [i915#265]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl8/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl8/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html * igt@kms_plane_lowres@pipe-a-tiling-x: - shard-glk: [PASS][21] -> [FAIL][22] ([i915#899]) [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-glk8/igt@kms_plane_lowres@pipe-a-tiling-x.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-glk3/igt@kms_plane_lowres@pipe-a-tiling-x.html * igt@kms_psr@psr2_cursor_mmap_cpu: - shard-iclb: [PASS][23] -> [SKIP][24] ([fdo#109441]) +3 similar issues [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb3/igt@kms_psr@psr2_cursor_mmap_cpu.html * igt@kms_setmode@basic: - shard-hsw: [PASS][25] -> [FAIL][26] ([i915#31]) [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-hsw7/igt@kms_setmode@basic.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-hsw1/igt@kms_setmode@basic.html * igt@prime_busy@hang-bsd2: - shard-iclb: [PASS][27] -> [SKIP][28] ([fdo#109276]) +17 similar issues [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb4/igt@prime_busy@hang-bsd2.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb3/igt@prime_busy@hang-bsd2.html #### Possible fixes #### * {igt@gem_ctx_persistence@engines-mixed-process@vecs0}: - shard-iclb: [FAIL][29] ([i915#679]) -> [PASS][30] [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb3/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb5/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html * {igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd}: - shard-skl: [FAIL][31] ([i915#679]) -> [PASS][32] [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl10/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl4/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd.html * {igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1}: - shard-skl: [INCOMPLETE][33] -> [PASS][34] [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl10/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl4/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html - shard-tglb: [FAIL][35] ([i915#679]) -> [PASS][36] [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-tglb8/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-tglb5/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html * {igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd2}: - shard-tglb: [INCOMPLETE][37] -> [PASS][38] [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-tglb8/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd2.html [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-tglb5/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd2.html * igt@gem_ctx_shared@exec-single-timeline-bsd: - shard-iclb: [SKIP][39] ([fdo#110841]) -> [PASS][40] [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb2/igt@gem_ctx_shared@exec-single-timeline-bsd.html [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb3/igt@gem_ctx_shared@exec-single-timeline-bsd.html * igt@gem_exec_schedule@pi-distinct-iova-bsd: - shard-iclb: [SKIP][41] ([i915#677]) -> [PASS][42] [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb2/igt@gem_exec_schedule@pi-distinct-iova-bsd.html [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb3/igt@gem_exec_schedule@pi-distinct-iova-bsd.html * igt@gem_exec_schedule@pi-shared-iova-bsd2: - shard-iclb: [SKIP][43] ([fdo#109276]) -> [PASS][44] +9 similar issues [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb5/igt@gem_exec_schedule@pi-shared-iova-bsd2.html [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb4/igt@gem_exec_schedule@pi-shared-iova-bsd2.html * igt@gem_exec_schedule@preempt-queue-bsd: - shard-iclb: [SKIP][45] ([fdo#112146]) -> [PASS][46] +1 similar issue [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd.html [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb3/igt@gem_exec_schedule@preempt-queue-bsd.html * igt@gem_exec_store@cachelines-vcs1: - shard-iclb: [SKIP][47] ([fdo#112080]) -> [PASS][48] +5 similar issues [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb6/igt@gem_exec_store@cachelines-vcs1.html [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb1/igt@gem_exec_store@cachelines-vcs1.html * igt@gem_partial_pwrite_pread@write-uncached: - shard-hsw: [FAIL][49] ([i915#694]) -> [PASS][50] +2 similar issues [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-hsw8/igt@gem_partial_pwrite_pread@write-uncached.html [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-hsw8/igt@gem_partial_pwrite_pread@write-uncached.html * igt@gem_ppgtt@flink-and-close-vma-leak: - shard-apl: [FAIL][51] ([i915#644]) -> [PASS][52] [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-apl4/igt@gem_ppgtt@flink-and-close-vma-leak.html [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-apl2/igt@gem_ppgtt@flink-and-close-vma-leak.html * igt@i915_pm_dc@dc6-dpms: - shard-iclb: [FAIL][53] ([i915#454]) -> [PASS][54] [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb5/igt@i915_pm_dc@dc6-dpms.html * igt@kms_busy@basic-modeset-pipe-b: - shard-snb: [SKIP][55] ([fdo#109271]) -> [PASS][56] +6 similar issues [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb6/igt@kms_busy@basic-modeset-pipe-b.html [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb2/igt@kms_busy@basic-modeset-pipe-b.html * igt@kms_cursor_crc@pipe-c-cursor-256x256-sliding: - shard-skl: [FAIL][57] ([i915#54]) -> [PASS][58] [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl4/igt@kms_cursor_crc@pipe-c-cursor-256x256-sliding.html [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl10/igt@kms_cursor_crc@pipe-c-cursor-256x256-sliding.html * igt@kms_flip@2x-flip-vs-expired-vblank: - shard-glk: [FAIL][59] ([i915#79]) -> [PASS][60] [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank.html [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank.html * igt@kms_flip@flip-vs-expired-vblank-interruptible: - shard-skl: [FAIL][61] ([i915#79]) -> [PASS][62] [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html * igt@kms_flip@plain-flip-fb-recreate-interruptible: - shard-skl: [FAIL][63] ([i915#34]) -> [PASS][64] [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl3/igt@kms_flip@plain-flip-fb-recreate-interruptible.html [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl5/igt@kms_flip@plain-flip-fb-recreate-interruptible.html * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-blt: - shard-skl: [FAIL][65] ([i915#49]) -> [PASS][66] [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl6/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-blt.html [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-blt.html * {igt@kms_hdr@bpc-switch-suspend}: - shard-skl: [FAIL][67] ([i915#1188]) -> [PASS][68] [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl7/igt@kms_hdr@bpc-switch-suspend.html [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl6/igt@kms_hdr@bpc-switch-suspend.html * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes: - shard-kbl: [DMESG-WARN][69] ([i915#180]) -> [PASS][70] +1 similar issue [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-kbl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-kbl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min: - shard-skl: [FAIL][71] ([fdo#108145]) -> [PASS][72] +1 similar issue [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html * igt@kms_psr@psr2_cursor_render: - shard-iclb: [SKIP][73] ([fdo#109441]) -> [PASS][74] +2 similar issues [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb1/igt@kms_psr@psr2_cursor_render.html [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb2/igt@kms_psr@psr2_cursor_render.html * igt@prime_mmap_coherency@ioctl-errors: - shard-hsw: [FAIL][75] ([i915#831]) -> [PASS][76] [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-hsw6/igt@prime_mmap_coherency@ioctl-errors.html [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-hsw5/igt@prime_mmap_coherency@ioctl-errors.html #### Warnings #### * igt@gem_ctx_isolation@vcs1-nonpriv-switch: - shard-iclb: [FAIL][77] ([IGT#28]) -> [SKIP][78] ([fdo#112080]) [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-iclb8/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html * igt@gem_tiled_blits@interruptible: - shard-hsw: [FAIL][79] ([i915#694]) -> [FAIL][80] ([i915#818]) [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-hsw6/igt@gem_tiled_blits@interruptible.html [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-hsw7/igt@gem_tiled_blits@interruptible.html * igt@gem_tiled_blits@normal: - shard-hsw: [FAIL][81] ([i915#818]) -> [FAIL][82] ([i915#694]) [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-hsw7/igt@gem_tiled_blits@normal.html [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-hsw1/igt@gem_tiled_blits@normal.html * igt@runner@aborted: - shard-snb: ([FAIL][83], [FAIL][84], [FAIL][85], [FAIL][86], [FAIL][87], [FAIL][88], [FAIL][89], [FAIL][90], [FAIL][91]) ([fdo#111870] / [i915#1077] / [i915#698]) -> ([FAIL][92], [FAIL][93], [FAIL][94], [FAIL][95], [FAIL][96], [FAIL][97], [FAIL][98], [FAIL][99], [FAIL][100], [FAIL][101]) ([fdo#111870] / [fdo#112162] / [i915#1077] / [i915#698]) [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb2/igt@runner@aborted.html [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb5/igt@runner@aborted.html [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb5/igt@runner@aborted.html [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb4/igt@runner@aborted.html [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb5/igt@runner@aborted.html [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb5/igt@runner@aborted.html [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb2/igt@runner@aborted.html [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb6/igt@runner@aborted.html [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7916/shard-snb4/igt@runner@aborted.html [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb6/igt@runner@aborted.html [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb4/igt@runner@aborted.html [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb2/igt@runner@aborted.html [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb4/igt@runner@aborted.html [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb6/igt@runner@aborted.html [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb4/igt@runner@aborted.html [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb2/igt@runner@aborted.html [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb6/igt@runner@aborted.html [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb6/igt@runner@aborted.html [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/shard-snb2/igt@runner@aborted.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [IGT#28]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/28 [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276 [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441 [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841 [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870 [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080 [fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146 [fdo#112162]: https://bugs.freedesktop.org/show_bug.cgi?id=112162 [i915#1077]: https://gitlab.freedesktop.org/drm/intel/issues/1077 [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188 [i915#168]: https://gitlab.freedesktop.org/drm/intel/issues/168 [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180 [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265 [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31 [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34 [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454 [i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49 [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54 [i915#644]: https://gitlab.freedesktop.org/drm/intel/issues/644 [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668 [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677 [i915#679]: https://gitlab.freedesktop.org/drm/intel/issues/679 [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694 [i915#698]: https://gitlab.freedesktop.org/drm/intel/issues/698 [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79 [i915#818]: https://gitlab.freedesktop.org/drm/intel/issues/818 [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82 [i915#831]: https://gitlab.freedesktop.org/drm/intel/issues/831 [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899 Participating hosts (10 -> 10) ------------------------------ No changes in participating hosts Build changes ------------- * CI: CI-20190529 -> None * Linux: CI_DRM_7916 -> Patchwork_16518 CI-20190529: 20190529 CI_DRM_7916: 6006a0cd15fdd07655be7c06729dd60b5135e42f @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5435: 2b6d4476dde53c363b8808ed9f0dd5547ac78641 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_16518: 4a714a53f220bb537aa11bf8404ccbb1bb5fbb7a @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16518/index.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-13 15:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-11 11:40 [Intel-gfx] [PATCH v1] drm/i915: Call intel_edp_init_connector only for eDP Stanislav Lisovskiy 2020-02-11 12:16 ` Imre Deak 2020-02-11 13:03 ` Jani Nikula 2020-02-11 13:33 ` Lisovskiy, Stanislav 2020-02-11 13:55 ` Jani Nikula 2020-02-11 14:03 ` Lisovskiy, Stanislav 2020-02-11 14:12 ` Jani Nikula 2020-02-11 14:27 ` Lisovskiy, Stanislav 2020-02-11 14:21 ` Imre Deak 2020-02-13 15:11 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).