All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: add missing condition for committing planes on crtc
@ 2016-05-04 11:13 Lionel Landwerlin
  2016-05-04 12:23 ` ✗ Fi.CI.BAT: failure for drm/i915: add missing condition for committing planes on crtc (rev2) Patchwork
  2016-05-09  7:37 ` [PATCH] drm/i915: add missing condition for committing planes on crtc Daniel Vetter
  0 siblings, 2 replies; 3+ messages in thread
From: Lionel Landwerlin @ 2016-05-04 11:13 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter

We are currently missing the color management update condition to
commit planes on crtc.

v2: add comment about moving the commit of color management registers
    to an async worker

Fixes: 20a34e78f0d7 (drm/i915: Update color management during vblank evasion.)
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45c218d..4936743 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13544,6 +13544,15 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
 	return false;
 }
 
+static bool needs_commit_planes_on_crtc(struct drm_crtc_state *crtc_state)
+{
+	/* TODO: drop the color management condition once committing
+	 * those registers has been moved to an async worker. */
+	return (crtc_state->planes_changed ||
+		crtc_state->color_mgmt_changed ||
+		to_intel_crtc_state(crtc_state)->update_pipe);
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13649,7 +13658,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 		bool modeset = needs_modeset(crtc->state);
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc->state);
-		bool update_pipe = !modeset && pipe_config->update_pipe;
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
@@ -13663,8 +13671,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    drm_atomic_get_existing_plane_state(state, crtc->primary))
 			intel_fbc_enable(intel_crtc);
 
-		if (crtc->state->active &&
-		    (crtc->state->planes_changed || update_pipe))
+		if (crtc->state->active && !modeset &&
+		    needs_commit_planes_on_crtc(crtc->state))
 			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
 
 		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
@@ -13974,6 +13982,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	if (modeset)
 		return;
 
+	/* TODO: Color management registers commit should be moved to
+	 * an async worker when we have them. */
 	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
 		intel_color_set_csc(crtc->state);
 		intel_color_load_luts(crtc->state);
-- 
2.8.0.rc3.226.g39d4020

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915: add missing condition for committing planes on crtc (rev2)
  2016-05-04 11:13 [PATCH] drm/i915: add missing condition for committing planes on crtc Lionel Landwerlin
@ 2016-05-04 12:23 ` Patchwork
  2016-05-09  7:37 ` [PATCH] drm/i915: add missing condition for committing planes on crtc Daniel Vetter
  1 sibling, 0 replies; 3+ messages in thread
From: Patchwork @ 2016-05-04 12:23 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: add missing condition for committing planes on crtc (rev2)
URL   : https://patchwork.freedesktop.org/series/5467/
State : failure

== Summary ==

Series 5467v2 drm/i915: add missing condition for committing planes on crtc
http://patchwork.freedesktop.org/api/1.0/series/5467/revisions/2/mbox/

Test drv_module_reload_basic:
                skip       -> DMESG-WARN (bdw-nuci7-2)
                pass       -> DMESG-WARN (bdw-ultra)
Test gem_busy:
        Subgroup basic-blt:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-bsd:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-bsd1:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-bsd2:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-blt:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-bsd:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-bsd1:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-bsd2:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-render:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-parallel-vebox:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-render:
                fail       -> PASS       (bdw-nuci7-2)
        Subgroup basic-vebox:
                fail       -> PASS       (bdw-nuci7-2)
Test gem_cs_tlb:
        Subgroup basic-default:
                skip       -> PASS       (bdw-nuci7-2)
Test gem_exec_flush:
        Subgroup basic-uc-pro-default-interruptible:
                pass       -> FAIL       (byt-nuc)
        Subgroup basic-uc-prw-default:
                fail       -> PASS       (bsw-nuc-2)
Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> DMESG-WARN (bdw-nuci7-2)
                pass       -> DMESG-WARN (bdw-ultra)
Test gem_sync:
        Subgroup basic-each:
                dmesg-fail -> PASS       (bdw-nuci7-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (skl-nuci5)
                pass       -> DMESG-WARN (bdw-ultra)
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (bsw-nuc-2)
                pass       -> FAIL       (skl-nuci5)
                pass       -> FAIL       (hsw-gt2)
                pass       -> DMESG-FAIL (bdw-ultra)
                pass       -> FAIL       (skl-i7k-2)
                pass       -> FAIL       (ivb-t430s)
                pass       -> FAIL       (byt-nuc)
                pass       -> FAIL       (snb-x220t)
                pass       -> FAIL       (hsw-brixbox)
                skip       -> FAIL       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-c:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
WARNING: Long output truncated
snb-dellxps failed to collect. IGT log at Patchwork_2135/snb-dellxps/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_2135/

9289da23b38812e3a351eaa1262b93536f81bfa8 drm-intel-nightly: 2016y-05m-04d-10h-35m-24s UTC integration manifest
8ca814e drm/i915: add missing condition for committing planes on crtc

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm/i915: add missing condition for committing planes on crtc
  2016-05-04 11:13 [PATCH] drm/i915: add missing condition for committing planes on crtc Lionel Landwerlin
  2016-05-04 12:23 ` ✗ Fi.CI.BAT: failure for drm/i915: add missing condition for committing planes on crtc (rev2) Patchwork
@ 2016-05-09  7:37 ` Daniel Vetter
  1 sibling, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2016-05-09  7:37 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: Jani Nikula, Daniel Vetter, intel-gfx

On Wed, May 04, 2016 at 12:13:39PM +0100, Lionel Landwerlin wrote:
> We are currently missing the color management update condition to
> commit planes on crtc.
> 
> v2: add comment about moving the commit of color management registers
>     to an async worker
> 
> Fixes: 20a34e78f0d7 (drm/i915: Update color management during vblank evasion.)
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 45c218d..4936743 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13544,6 +13544,15 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>  	return false;
>  }
>  
> +static bool needs_commit_planes_on_crtc(struct drm_crtc_state *crtc_state)
> +{
> +	/* TODO: drop the color management condition once committing
> +	 * those registers has been moved to an async worker. */
> +	return (crtc_state->planes_changed ||
> +		crtc_state->color_mgmt_changed ||
> +		to_intel_crtc_state(crtc_state)->update_pipe);
> +}

I think it'd be cleaner (as in related logic pulled together) and more in
spirit of the atomic framework if you set planes_changed when you notice
that color mgm state needs an update. That makes it clear that on intel hw
we do such changes with a plain flip (well later on maybe we need a bool
for the vblank worker). Other hw might need a full modeset.

Adding more conditions like this to the commit code makes it imo really
fragile and hard to understand. At least imo computing how to commit state
is best placed next to when we compute what changed.

All the *_changed variables computed by the helpers/core are meant to be
changeable by the driver (assuming it knows what it's doing), and in
general can be upgraded to true without causing any trouble (except maybe
running some code that doesn't necessarily need to be run).
-Daniel

> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13649,7 +13658,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		bool modeset = needs_modeset(crtc->state);
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc->state);
> -		bool update_pipe = !modeset && pipe_config->update_pipe;
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
> @@ -13663,8 +13671,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		    drm_atomic_get_existing_plane_state(state, crtc->primary))
>  			intel_fbc_enable(intel_crtc);
>  
> -		if (crtc->state->active &&
> -		    (crtc->state->planes_changed || update_pipe))
> +		if (crtc->state->active && !modeset &&
> +		    needs_commit_planes_on_crtc(crtc->state))
>  			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
>  
>  		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
> @@ -13974,6 +13982,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	if (modeset)
>  		return;
>  
> +	/* TODO: Color management registers commit should be moved to
> +	 * an async worker when we have them. */
>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
>  		intel_color_load_luts(crtc->state);
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-05-09  7:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-04 11:13 [PATCH] drm/i915: add missing condition for committing planes on crtc Lionel Landwerlin
2016-05-04 12:23 ` ✗ Fi.CI.BAT: failure for drm/i915: add missing condition for committing planes on crtc (rev2) Patchwork
2016-05-09  7:37 ` [PATCH] drm/i915: add missing condition for committing planes on crtc Daniel Vetter

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.