Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Fri, 14 Feb 2020, "Sarvela, Tomi P" wrote: >> From: Jani Nikula >> >> On Mon, 10 Feb 2020, Arkadiusz Hiler wrote: >> > As of the 3 days worth of queued shards: >> > >> > I agree that this is unacceptable, but we can do only so much from the >> > CI/infra side. The time has been creeping up steadily over the last year >> > or so and the machines are not getting any faster. >> >> I am *not* trying to say that it's all your fault and you need to >> provide all results faster for the ever-increasing firehose of incoming >> patches. >> >> I'd like to pose the question, what would all this look like if we made >> it a hard requirement that we need a go/no-go decision on every patch >> series within 24 hours? I emphasize that I don't mean full results in 24 >> hours. Given all the other constraints, how could we provide as much >> useful information as possible within 24 hours to make a decision? >> >> In another thread I said, we've shifted a bit from review being the >> bottle neck to shard runs being the bottle neck. It's still much more >> likely that a patch will change due to review feedback instead of shard >> run results. Half a dozen rounds of review ping pong directly leads to >> half a dozen rounds of mostly unnecessary testing. I would not outright >> dismiss only running full igt on reviewed/acked patches. > > This is actually a good idea. In practice, the shards are swamped by the > amount of builds today, and the throughput has been close to 1/h a long > time, even with work ongoing to prune or tighten stupidest IGT tests. > > We could make the shard run requirements stricter: in addition to passing > BAT it would need some amount of Acks. Patchwork already collects them. Of course, patchwork isn't accurate in picking acks/reviews, but I don't think it has to be. Err on the side of testing, and provide a way to start shard runs manually, also because sometimes you do want the results ASAP on v1. (On that note, would be nice if people could *remove* their patch series from the shard queueu too.) > Another idea has been moving the serialized shard run queue to something > that can handle reordering: trybots can be moved after everything else. This > doesn't affect to the shard queue length though, if we still want to test > everything. Next we'll be figuring out a fair scheduler that does not starve the trybot queue. ;) >> Additionally, there are smaller optimizations to be made (obviously all >> depending on developer bandwidth to implement this stuff), such as >> identifying patches that don't change the resulting binary >> (comment/documentation/whitespace changes), and only running build >> testing on them. > > This idea has been floating around, and would help in 5% changes or so > (which is still noticeable: 1-2 more builds / day tested instead of queued). > > Just need a good diff checker that says "text changes only, skip it". It's probably not as trivial as it initially sounds, but gut feeling says that it's also not a problem that nobody has tried to solve 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
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
> From: Jani Nikula > > On Mon, 10 Feb 2020, Arkadiusz Hiler wrote: > > As of the 3 days worth of queued shards: > > > > I agree that this is unacceptable, but we can do only so much from the > > CI/infra side. The time has been creeping up steadily over the last year > > or so and the machines are not getting any faster. > > I am *not* trying to say that it's all your fault and you need to > provide all results faster for the ever-increasing firehose of incoming > patches. > > I'd like to pose the question, what would all this look like if we made > it a hard requirement that we need a go/no-go decision on every patch > series within 24 hours? I emphasize that I don't mean full results in 24 > hours. Given all the other constraints, how could we provide as much > useful information as possible within 24 hours to make a decision? > > In another thread I said, we've shifted a bit from review being the > bottle neck to shard runs being the bottle neck. It's still much more > likely that a patch will change due to review feedback instead of shard > run results. Half a dozen rounds of review ping pong directly leads to > half a dozen rounds of mostly unnecessary testing. I would not outright > dismiss only running full igt on reviewed/acked patches. This is actually a good idea. In practice, the shards are swamped by the amount of builds today, and the throughput has been close to 1/h a long time, even with work ongoing to prune or tighten stupidest IGT tests. We could make the shard run requirements stricter: in addition to passing BAT it would need some amount of Acks. Patchwork already collects them. Another idea has been moving the serialized shard run queue to something that can handle reordering: trybots can be moved after everything else. This doesn't affect to the shard queue length though, if we still want to test everything. > Additionally, there are smaller optimizations to be made (obviously all > depending on developer bandwidth to implement this stuff), such as > identifying patches that don't change the resulting binary > (comment/documentation/whitespace changes), and only running build > testing on them. This idea has been floating around, and would help in 5% changes or so (which is still noticeable: 1-2 more builds / day tested instead of queued). Just need a good diff checker that says "text changes only, skip it". Tomi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Mon, 10 Feb 2020, Arkadiusz Hiler wrote: > As of the 3 days worth of queued shards: > > I agree that this is unacceptable, but we can do only so much from the > CI/infra side. The time has been creeping up steadily over the last year > or so and the machines are not getting any faster. I am *not* trying to say that it's all your fault and you need to provide all results faster for the ever-increasing firehose of incoming patches. I'd like to pose the question, what would all this look like if we made it a hard requirement that we need a go/no-go decision on every patch series within 24 hours? I emphasize that I don't mean full results in 24 hours. Given all the other constraints, how could we provide as much useful information as possible within 24 hours to make a decision? In another thread I said, we've shifted a bit from review being the bottle neck to shard runs being the bottle neck. It's still much more likely that a patch will change due to review feedback instead of shard run results. Half a dozen rounds of review ping pong directly leads to half a dozen rounds of mostly unnecessary testing. I would not outright dismiss only running full igt on reviewed/acked patches. Additionally, there are smaller optimizations to be made (obviously all depending on developer bandwidth to implement this stuff), such as identifying patches that don't change the resulting binary (comment/documentation/whitespace changes), and only running build testing on them. 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
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Mon, Feb 10, 2020 at 01:43:47PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, 2020-02-10 at 13:32 +0200, Jani Nikula wrote:
> > On Wed, 05 Feb 2020, Jani Nikula wrote:
> > > Commit 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> > > encoders on DDI platforms") pushed pipe and vblank enable to
> > > encoders on
> > > DDI platforms, however it missed the DP MST encoder. Fix it.
> > >
> > > Fixes: 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> > > encoders on DDI platforms")
> > > Cc: Vandita Kulkarni
> > > Cc: Ville Syrjala
> > > Reported-by: Stanislav Lisovskiy
> > > Signed-off-by: Jani Nikula
> >
> > Thanks for the reviews and testing, pushed to dinq.
> >
> > I don't usually cut corners, but I've made an exception and pushed
> > this without full IGT results.
> >
> > It's been 5 days since the patch was posted, the sharded run has
> > fallen between the cracks, and the queue is currently about three
> > days. IMHO it's intolerable for any patch, but especially so for a
> > regression fix that was posted within hours of the bug report.
>
> Absolutely agree, since we already had a regression, it's pointless
> now to wait longer with such a trivial fix. We are anyway in a bad
> situation now, checking also some other MST issues and having to apply
> this patch manually first in order to get at least this issue ruled
> out.
>
> Stan
As of why it was silently dropped:
We poke patchwork to check whether there is a newer version of a given
series. If there is we won't waste time on running the older one through
shards.
This bit looks more or less like this:
RES="$(curl -q
https://patchwork.freedesktop.org/api/1.0/series/$SER/revisions/$(( $REV + 1
))/ 2>/dev/null)"
[[ "$RES" = "" || "$RES" = *"ot found"* ]] || exit 1
If there is a network issue and curl exits with non-zero exit status
this aborts the shards because of `set -e`, which is what has happened:
+++ curl -q
https://patchwork.freedesktop.org/api/1.0/series/73006/revisions/2/
++ RES=
Build step 'Execute shell' marked build as failure
So a network issue + not robust enough bash script is the cause.
I have fixed the logic there to account for this and in case of network
errors we just go ahead with testing. Thanks for rising this up.
As of the 3 days worth of queued shards:
I agree that this is unacceptable, but we can do only so much from the
CI/infra side. The time has been creeping up steadily over the last year
or so and the machines are not getting any faster.
We are currently sitting on ~58min for a run and Tomi has already done a
lot of in terms of optimization. The overhead is as minimal as it can be
and there is some logic tracking the test execution times and doing
random but balanced test distribution.
We are also considering introducing hard limits on subtest execution
times and hunting down the tests that are exceeding this.
On IGT side there was a recent introduction of dynamic subtest which
should help with time wasted on some of the skips, and I am working on a
more reliable skips for multiple mode testing (currently one subtest =
one execv) but without optimizing the test cases we won't shave off much
time.
--
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Mon, 2020-02-10 at 13:32 +0200, Jani Nikula wrote:
> On Wed, 05 Feb 2020, Jani Nikula wrote:
> > Commit 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> > encoders on DDI platforms") pushed pipe and vblank enable to
> > encoders on
> > DDI platforms, however it missed the DP MST encoder. Fix it.
> >
> > Fixes: 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> > encoders on DDI platforms")
> > Cc: Vandita Kulkarni
> > Cc: Ville Syrjala
> > Reported-by: Stanislav Lisovskiy
> > Signed-off-by: Jani Nikula
>
> Thanks for the reviews and testing, pushed to dinq.
>
> I don't usually cut corners, but I've made an exception and pushed
> this
> without full IGT results.
>
> It's been 5 days since the patch was posted, the sharded run has
> fallen
> between the cracks, and the queue is currently about three days. IMHO
> it's intolerable for any patch, but especially so for a regression
> fix
> that was posted within hours of the bug report.
Absolutely agree, since we already had a regression, it's pointless
now to wait longer with such a trivial fix. We are anyway in a bad
situation now, checking also some other MST issues and having to apply
this patch manually first in order to get at least this issue ruled
out.
Stan
>
> BR,
> Jani.
>
>
> > ---
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index b8aee506d595..9cd59141953d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -491,6 +491,12 @@ static void intel_mst_enable_dp(struct
> > intel_encoder *encoder,
> > struct intel_dp *intel_dp = _dig_port->dp;
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >
> > + drm_WARN_ON(_priv->drm, pipe_config->has_pch_encoder);
> > +
> > + intel_enable_pipe(pipe_config);
> > +
> > + intel_crtc_vblank_on(pipe_config);
> > +
> > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >
> > if (intel_de_wait_for_set(dev_priv, intel_dp-
> > >regs.dp_tp_status,
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Wed, 05 Feb 2020, Jani Nikula wrote:
> Commit 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> encoders on DDI platforms") pushed pipe and vblank enable to encoders on
> DDI platforms, however it missed the DP MST encoder. Fix it.
>
> Fixes: 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to encoders
> on DDI platforms")
> Cc: Vandita Kulkarni
> Cc: Ville Syrjala
> Reported-by: Stanislav Lisovskiy
> Signed-off-by: Jani Nikula
Thanks for the reviews and testing, pushed to dinq.
I don't usually cut corners, but I've made an exception and pushed this
without full IGT results.
It's been 5 days since the patch was posted, the sharded run has fallen
between the cracks, and the queue is currently about three days. IMHO
it's intolerable for any patch, but especially so for a regression fix
that was posted within hours of the bug report.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index b8aee506d595..9cd59141953d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -491,6 +491,12 @@ static void intel_mst_enable_dp(struct intel_encoder
> *encoder,
> struct intel_dp *intel_dp = _dig_port->dp;
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>
> + drm_WARN_ON(_priv->drm, pipe_config->has_pch_encoder);
> +
> + intel_enable_pipe(pipe_config);
> +
> + intel_crtc_vblank_on(pipe_config);
> +
> DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
On Wed, 2020-02-05 at 10:29 +0200, Jani Nikula wrote:
> Commit 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> encoders on DDI platforms") pushed pipe and vblank enable to encoders
> on
> DDI platforms, however it missed the DP MST encoder. Fix it.
>
> Fixes: 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> encoders on DDI platforms")
> Cc: Vandita Kulkarni
> Cc: Ville Syrjala
> Reported-by: Stanislav Lisovskiy
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 ++
> 1 file changed, 6 insertions(+)
Checked, seems to fix my displays, so
Reviewed-by: Stanislav Lisovskiy
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index b8aee506d595..9cd59141953d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -491,6 +491,12 @@ static void intel_mst_enable_dp(struct
> intel_encoder *encoder,
> struct intel_dp *intel_dp = _dig_port->dp;
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>
> + drm_WARN_ON(_priv->drm, pipe_config->has_pch_encoder);
> +
> + intel_enable_pipe(pipe_config);
> +
> + intel_crtc_vblank_on(pipe_config);
> +
> DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> if (intel_de_wait_for_set(dev_priv, intel_dp-
> >regs.dp_tp_status,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/mst: fix pipe and vblank enable
> -Original Message-
> From: Jani Nikula
> Sent: Wednesday, February 5, 2020 2:00 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani ; Kulkarni, Vandita
> ; Ville Syrjala ;
> Lisovskiy, Stanislav
> Subject: [PATCH] drm/i915/mst: fix pipe and vblank enable
>
> Commit 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to
> encoders on DDI platforms") pushed pipe and vblank enable to encoders on DDI
> platforms, however it missed the DP MST encoder. Fix it.
>
> Fixes: 21fd23ac222f ("drm/i915: move pipe, pch and vblank enable to encoders
> on DDI platforms")
> Cc: Vandita Kulkarni
> Cc: Ville Syrjala
> Reported-by: Stanislav Lisovskiy
> Signed-off-by: Jani Nikula
Looks good to me.
Reviewed-by: Vandita Kulkarni
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index b8aee506d595..9cd59141953d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -491,6 +491,12 @@ static void intel_mst_enable_dp(struct intel_encoder
> *encoder,
> struct intel_dp *intel_dp = _dig_port->dp;
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>
> + drm_WARN_ON(_priv->drm, pipe_config->has_pch_encoder);
> +
> + intel_enable_pipe(pipe_config);
> +
> + intel_crtc_vblank_on(pipe_config);
> +
> DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
> --
> 2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
