Re: [Intel-gfx] [PATCH 32/42] drm/i915: Calculate haswell plane workaround.
On Tue, May 12, 2015 at 04:05:24PM +0200, Maarten Lankhorst wrote:
> Op 12-05-15 om 11:43 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:25:08PM +0200, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 109
> >> +--
> >> drivers/gpu/drm/i915/intel_drv.h | 2 +
> >> 2 files changed, 81 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 7bc78b49f9f4..7a79659dca00 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc
> >> *crtc)
> >>return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
> >> }
> >>
> >> -/*
> >> - * This implements the workaround described in the "notes" section of the
> >> mode
> >> - * set sequence documentation. When going from no pipes or single pipe to
> >> - * multiple pipes, and planes are enabled after the pipe, we need to wait
> >> at
> >> - * least 2 vblanks on the first pipe before enabling planes on the second
> >> pipe.
> >> - */
> >> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
> >> -{
> >> - struct drm_device *dev = crtc->base.dev;
> >> - struct intel_crtc *crtc_it, *other_active_crtc = NULL;
> >> -
> >> - /* We want to get the other_active_crtc only if there's only 1 other
> >> - * active crtc. */
> >> - for_each_intel_crtc(dev, crtc_it) {
> >> - if (!crtc_it->active || crtc_it == crtc)
> >> - continue;
> >> -
> >> - if (other_active_crtc)
> >> - return;
> >> -
> >> - other_active_crtc = crtc_it;
> >> - }
> >> - if (!other_active_crtc)
> >> - return;
> >> -
> >> - intel_wait_for_vblank(dev, other_active_crtc->pipe);
> >> - intel_wait_for_vblank(dev, other_active_crtc->pipe);
> >> -}
> >> -
> >> static void haswell_crtc_enable(struct drm_crtc *crtc)
> >> {
> >>struct drm_device *dev = crtc->dev;
> >> @@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc
> >> *crtc)
> >>
> >>/* If we change the relative order between pipe/planes enabling, we need
> >> * to change the workaround. */
> >> - haswell_mode_set_planes_workaround(intel_crtc);
> >> + if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
> >> + intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> >> + intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> >> + }
> >> }
> >>
> >> static void ironlake_pfit_disable(struct intel_crtc *crtc)
> >> @@ -12147,6 +12121,74 @@ done:
> >>return ret;
> >> }
> >>
> >> +/*
> >> + * This implements the workaround described in the "notes" section of the
> >> mode
> >> + * set sequence documentation. When going from no pipes or single pipe to
> >> + * multiple pipes, and planes are enabled after the pipe, we need to wait
> >> at
> >> + * least 2 vblanks on the first pipe before enabling planes on the second
> >> pipe.
> >> + */
> >> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state
> >> *state)
> >> +{
> >> + struct drm_crtc *crtc;
> >> + struct drm_crtc_state *crtc_state;
> >> + struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state =
> >> NULL;
> >> + struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
> >> + int enabled_crtcs = 0, ret, i;
> >> +
> >> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >> + struct intel_crtc_state *pipe_config =
> >> + to_intel_crtc_state(crtc_state);
> >> +
> >> + pipe_config->hsw_workaround_pipe = INVALID_PIPE;
> > This kind of state resetting should be done in duplicate_state.
> The v2 of this patch uses intel_crtc->atomic.hsw_workaround pipe, which would
> work better. :)
> >> +
> >> + if (!crtc_state->active)
> >> + continue;
> >> +
> >> + if (!needs_modeset(crtc_state)) {
> >> + enabled_crtcs++;
> >> + enabled_crtc = to_intel_crtc(crtc);
> >> + continue;
> >> + }
> >> +
> >> + if (first_crtc) {
> >> + other_crtc_state = pipe_config;
> >> + break;
> >> + }
> >> + first_crtc = to_intel_crtc(crtc);
> >> + first_crtc_state = pipe_config;
> >> + }
> >> +
> >> + /* No workaround needed? */
> >> + if (!first_crtc || enabled_crtcs > 1)
> >> + return 0;
> >> +
> >> + for_each_intel_crtc(state->dev, intel_crtc) {
> >> + if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
> >> + continue;
> >> +
> >> + ret = drm_modeset_lock(&intel_crtc->base.mutex,
> >> + state->acquire_ctx);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!intel_crtc->base.s
Re: [Intel-gfx] [PATCH 32/42] drm/i915: Calculate haswell plane workaround.
Op 12-05-15 om 11:43 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:25:08PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 109
>> +--
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 2 files changed, 81 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 7bc78b49f9f4..7a79659dca00 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc
>> *crtc)
>> return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>> }
>>
>> -/*
>> - * This implements the workaround described in the "notes" section of the
>> mode
>> - * set sequence documentation. When going from no pipes or single pipe to
>> - * multiple pipes, and planes are enabled after the pipe, we need to wait at
>> - * least 2 vblanks on the first pipe before enabling planes on the second
>> pipe.
>> - */
>> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>> -{
>> -struct drm_device *dev = crtc->base.dev;
>> -struct intel_crtc *crtc_it, *other_active_crtc = NULL;
>> -
>> -/* We want to get the other_active_crtc only if there's only 1 other
>> - * active crtc. */
>> -for_each_intel_crtc(dev, crtc_it) {
>> -if (!crtc_it->active || crtc_it == crtc)
>> -continue;
>> -
>> -if (other_active_crtc)
>> -return;
>> -
>> -other_active_crtc = crtc_it;
>> -}
>> -if (!other_active_crtc)
>> -return;
>> -
>> -intel_wait_for_vblank(dev, other_active_crtc->pipe);
>> -intel_wait_for_vblank(dev, other_active_crtc->pipe);
>> -}
>> -
>> static void haswell_crtc_enable(struct drm_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->dev;
>> @@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>
>> /* If we change the relative order between pipe/planes enabling, we need
>> * to change the workaround. */
>> -haswell_mode_set_planes_workaround(intel_crtc);
>> +if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
>> +intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
>> +intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
>> +}
>> }
>>
>> static void ironlake_pfit_disable(struct intel_crtc *crtc)
>> @@ -12147,6 +12121,74 @@ done:
>> return ret;
>> }
>>
>> +/*
>> + * This implements the workaround described in the "notes" section of the
>> mode
>> + * set sequence documentation. When going from no pipes or single pipe to
>> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
>> + * least 2 vblanks on the first pipe before enabling planes on the second
>> pipe.
>> + */
>> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state
>> *state)
>> +{
>> +struct drm_crtc *crtc;
>> +struct drm_crtc_state *crtc_state;
>> +struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state =
>> NULL;
>> +struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
>> +int enabled_crtcs = 0, ret, i;
>> +
>> +for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +struct intel_crtc_state *pipe_config =
>> +to_intel_crtc_state(crtc_state);
>> +
>> +pipe_config->hsw_workaround_pipe = INVALID_PIPE;
> This kind of state resetting should be done in duplicate_state.
The v2 of this patch uses intel_crtc->atomic.hsw_workaround pipe, which would
work better. :)
>> +
>> +if (!crtc_state->active)
>> +continue;
>> +
>> +if (!needs_modeset(crtc_state)) {
>> +enabled_crtcs++;
>> +enabled_crtc = to_intel_crtc(crtc);
>> +continue;
>> +}
>> +
>> +if (first_crtc) {
>> +other_crtc_state = pipe_config;
>> +break;
>> +}
>> +first_crtc = to_intel_crtc(crtc);
>> +first_crtc_state = pipe_config;
>> +}
>> +
>> +/* No workaround needed? */
>> +if (!first_crtc || enabled_crtcs > 1)
>> +return 0;
>> +
>> +for_each_intel_crtc(state->dev, intel_crtc) {
>> +if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
>> +continue;
>> +
>> +ret = drm_modeset_lock(&intel_crtc->base.mutex,
>> + state->acquire_ctx);
>> +if (ret)
>> +return ret;
>> +
>> +if (!intel_crtc->base.state->active)
>> +continue;
> Imo just unconditionally acquire the crtc state, there' shouldn't be any
> harm in that (as long as we leave mode/active/planes_changed untouched).
Probably.
>> +
>> +
Re: [Intel-gfx] [PATCH 32/42] drm/i915: Calculate haswell plane workaround.
On Mon, May 11, 2015 at 04:25:08PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_display.c | 109
> +--
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 2 files changed, 81 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7bc78b49f9f4..7a79659dca00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc
> *crtc)
> return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
> }
>
> -/*
> - * This implements the workaround described in the "notes" section of the
> mode
> - * set sequence documentation. When going from no pipes or single pipe to
> - * multiple pipes, and planes are enabled after the pipe, we need to wait at
> - * least 2 vblanks on the first pipe before enabling planes on the second
> pipe.
> - */
> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
> -{
> - struct drm_device *dev = crtc->base.dev;
> - struct intel_crtc *crtc_it, *other_active_crtc = NULL;
> -
> - /* We want to get the other_active_crtc only if there's only 1 other
> - * active crtc. */
> - for_each_intel_crtc(dev, crtc_it) {
> - if (!crtc_it->active || crtc_it == crtc)
> - continue;
> -
> - if (other_active_crtc)
> - return;
> -
> - other_active_crtc = crtc_it;
> - }
> - if (!other_active_crtc)
> - return;
> -
> - intel_wait_for_vblank(dev, other_active_crtc->pipe);
> - intel_wait_for_vblank(dev, other_active_crtc->pipe);
> -}
> -
> static void haswell_crtc_enable(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>
> /* If we change the relative order between pipe/planes enabling, we need
>* to change the workaround. */
> - haswell_mode_set_planes_workaround(intel_crtc);
> + if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
> + intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> + intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> + }
> }
>
> static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -12147,6 +12121,74 @@ done:
> return ret;
> }
>
> +/*
> + * This implements the workaround described in the "notes" section of the
> mode
> + * set sequence documentation. When going from no pipes or single pipe to
> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
> + * least 2 vblanks on the first pipe before enabling planes on the second
> pipe.
> + */
> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state =
> NULL;
> + struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
> + int enabled_crtcs = 0, ret, i;
> +
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc_state);
> +
> + pipe_config->hsw_workaround_pipe = INVALID_PIPE;
This kind of state resetting should be done in duplicate_state.
> +
> + if (!crtc_state->active)
> + continue;
> +
> + if (!needs_modeset(crtc_state)) {
> + enabled_crtcs++;
> + enabled_crtc = to_intel_crtc(crtc);
> + continue;
> + }
> +
> + if (first_crtc) {
> + other_crtc_state = pipe_config;
> + break;
> + }
> + first_crtc = to_intel_crtc(crtc);
> + first_crtc_state = pipe_config;
> + }
> +
> + /* No workaround needed? */
> + if (!first_crtc || enabled_crtcs > 1)
> + return 0;
> +
> + for_each_intel_crtc(state->dev, intel_crtc) {
> + if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
> + continue;
> +
> + ret = drm_modeset_lock(&intel_crtc->base.mutex,
> +state->acquire_ctx);
> + if (ret)
> + return ret;
> +
> + if (!intel_crtc->base.state->active)
> + continue;
Imo just unconditionally acquire the crtc state, there' shouldn't be any
harm in that (as long as we leave mode/active/planes_changed untouched).
> +
> + /* 2 enabled crtcs means no need for w/a */
> + if (++enabled_crtcs >= 2)
> + return 0;
> +
> + enabled_crtc = intel_crtc;
> + }
> +
> + if (enabled_crtcs == 1
[Intel-gfx] [PATCH 32/42] drm/i915: Calculate haswell plane workaround.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_display.c | 109 +--
drivers/gpu/drm/i915/intel_drv.h | 2 +
2 files changed, 81 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7bc78b49f9f4..7a79659dca00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc
*crtc)
return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
}
-/*
- * This implements the workaround described in the "notes" section of the mode
- * set sequence documentation. When going from no pipes or single pipe to
- * multiple pipes, and planes are enabled after the pipe, we need to wait at
- * least 2 vblanks on the first pipe before enabling planes on the second pipe.
- */
-static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
-{
- struct drm_device *dev = crtc->base.dev;
- struct intel_crtc *crtc_it, *other_active_crtc = NULL;
-
- /* We want to get the other_active_crtc only if there's only 1 other
-* active crtc. */
- for_each_intel_crtc(dev, crtc_it) {
- if (!crtc_it->active || crtc_it == crtc)
- continue;
-
- if (other_active_crtc)
- return;
-
- other_active_crtc = crtc_it;
- }
- if (!other_active_crtc)
- return;
-
- intel_wait_for_vblank(dev, other_active_crtc->pipe);
- intel_wait_for_vblank(dev, other_active_crtc->pipe);
-}
-
static void haswell_crtc_enable(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
@@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
/* If we change the relative order between pipe/planes enabling, we need
* to change the workaround. */
- haswell_mode_set_planes_workaround(intel_crtc);
+ if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
+ intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
+ intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
+ }
}
static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -12147,6 +12121,74 @@ done:
return ret;
}
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state =
NULL;
+ struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
+ int enabled_crtcs = 0, ret, i;
+
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_crtc_state *pipe_config =
+ to_intel_crtc_state(crtc_state);
+
+ pipe_config->hsw_workaround_pipe = INVALID_PIPE;
+
+ if (!crtc_state->active)
+ continue;
+
+ if (!needs_modeset(crtc_state)) {
+ enabled_crtcs++;
+ enabled_crtc = to_intel_crtc(crtc);
+ continue;
+ }
+
+ if (first_crtc) {
+ other_crtc_state = pipe_config;
+ break;
+ }
+ first_crtc = to_intel_crtc(crtc);
+ first_crtc_state = pipe_config;
+ }
+
+ /* No workaround needed? */
+ if (!first_crtc || enabled_crtcs > 1)
+ return 0;
+
+ for_each_intel_crtc(state->dev, intel_crtc) {
+ if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
+ continue;
+
+ ret = drm_modeset_lock(&intel_crtc->base.mutex,
+ state->acquire_ctx);
+ if (ret)
+ return ret;
+
+ if (!intel_crtc->base.state->active)
+ continue;
+
+ /* 2 enabled crtcs means no need for w/a */
+ if (++enabled_crtcs >= 2)
+ return 0;
+
+ enabled_crtc = intel_crtc;
+ }
+
+ if (enabled_crtcs == 1)
+ first_crtc_state->hsw_workaround_pipe = enabled_crtc->pipe;
+ else if (other_crtc_state)
+ other_crtc_state->hsw_workaround_pipe = first_crtc->pipe;
+
+ return 0;
+}
+
/* Code that should eventually be part of atomic_check() */
static int __intel_set_mode_checks(struct drm_atomic_state *state)
{
@@ -12158,6 +12200,13 @@ static int __intel_set_mode_checks(struct
drm_
