* [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 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
* 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
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.