[PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Daniel Vetter]

Since universal planes the primary plane might not be around, and it's
kinda silly to restrict the pipe bpp to the primary plane if we might
end up displaying a 10bpc video overlay. And with atomic we might very
well enable a pipe without a primary plane. So just use the platform
max as a starting point and then restrict appropriately.

Of course this is all still a bit moot as long as we artificially
compress everything to max 8bpc because we don't use the hi-bpc gamma
tables.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 61 +++++++-----------------------------
 1 file changed, 12 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 11b958a56515..b1afd7d5e28c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5869,14 +5869,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
 		return -EINVAL;
 
-	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) {
-		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
-	} else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8*3) {
-		/* only a 8bpc pipe, with 6bpc dither through the panel fitter
-		 * for lvds. */
-		pipe_config->pipe_bpp = 8*3;
-	}
-
 	if (HAS_IPS(dev))
 		hsw_compute_ips_config(crtc, pipe_config);
 
@@ -10503,7 +10495,6 @@ connected_sink_compute_bpp(struct intel_connector *connector,
 
 static int
 compute_baseline_pipe_bpp(struct intel_crtc *crtc,
-			  struct drm_framebuffer *fb,
 			  struct intel_crtc_state *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -10511,41 +10502,13 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 	struct intel_connector *connector;
 	int bpp, i;
 
-	switch (fb->pixel_format) {
-	case DRM_FORMAT_C8:
-		bpp = 8*3; /* since we go through a colormap */
-		break;
-	case DRM_FORMAT_XRGB1555:
-	case DRM_FORMAT_ARGB1555:
-		/* checked in intel_framebuffer_init already */
-		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
-			return -EINVAL;
-	case DRM_FORMAT_RGB565:
-		bpp = 6*3; /* min is 18bpp */
-		break;
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_ABGR8888:
-		/* checked in intel_framebuffer_init already */
-		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
-			return -EINVAL;
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_ARGB8888:
-		bpp = 8*3;
-		break;
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_ARGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_ABGR2101010:
-		/* checked in intel_framebuffer_init already */
-		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
-			return -EINVAL;
+	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)))
 		bpp = 10*3;
-		break;
-	/* TODO: gen4+ supports 16 bpc floating point, too. */
-	default:
-		DRM_DEBUG_KMS("unsupported depth\n");
-		return -EINVAL;
-	}
+	else if (INTEL_INFO(dev)->gen >= 5)
+		bpp = 12*3;
+	else
+		bpp = 8*3;
+
 
 	pipe_config->pipe_bpp = bpp;
 
@@ -10756,7 +10719,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *pipe_config;
-	int plane_bpp, ret = -EINVAL;
+	int base_bpp, ret = -EINVAL;
 	int i;
 	bool retry = true;
 
@@ -10801,9 +10764,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 * plane pixel format and any sink constraints into account. Returns the
 	 * source plane bpp so that dithering can be selected on mismatches
 	 * after encoders and crtc also have had their say. */
-	plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
-					      fb, pipe_config);
-	if (plane_bpp < 0)
+	base_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
+					     pipe_config);
+	if (base_bpp < 0)
 		goto fail;
 
 	/*
@@ -10871,9 +10834,9 @@ encoder_retry:
 		goto encoder_retry;
 	}
 
-	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
+	pipe_config->dither = pipe_config->pipe_bpp != base_bpp;
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
-		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
+		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
 	return pipe_config;
 fail:
-- 
2.1.0

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

[PATCH 2/3] drm/i915: Drop unecessary fb arguments from function signatures

[Daniel Vetter]

This is a separate patch to simplify conflict handling with other
ongoing atomic work.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1afd7d5e28c..cdf21df8c69c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10711,7 +10711,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 
 static struct intel_crtc_state *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
-			  struct drm_framebuffer *fb,
 			  struct drm_display_mode *mode,
 			  struct drm_atomic_state *state)
 {
@@ -11514,7 +11513,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 static struct intel_crtc_state *
 intel_modeset_compute_config(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
-			     struct drm_framebuffer *fb,
 			     struct drm_atomic_state *state,
 			     unsigned *modeset_pipes,
 			     unsigned *prepare_pipes,
@@ -11552,7 +11550,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 		if (WARN_ON(&intel_crtc->base != crtc))
 			continue;
 
-		pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
+		pipe_config = intel_modeset_pipe_config(crtc, mode, state);
 		if (IS_ERR(pipe_config))
 			return pipe_config;
 
@@ -11758,7 +11756,7 @@ static int intel_set_mode(struct drm_crtc *crtc,
 	unsigned modeset_pipes, prepare_pipes, disable_pipes;
 	int ret = 0;
 
-	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
+	pipe_config = intel_modeset_compute_config(crtc, mode, state,
 						   &modeset_pipes,
 						   &prepare_pipes,
 						   &disable_pipes);
@@ -12216,7 +12214,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		goto fail;
 
 	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
-						   set->fb, state,
+						   state,
 						   &modeset_pipes,
 						   &prepare_pipes,
 						   &disable_pipes);
-- 
2.1.0

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

[PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Daniel Vetter]

It's a silly thing to do and surprises driver writers. Most likely
this did already blow up for exynos.

It's also a silly thing to change plane state when it's off, but fbdev
is silly (it does an unconditional modeset over all planes). And
userspace can be evil. So I think we need this.

With this check in the helpers we can remove the one in i915 code for
the same conditions (becuase ->crtc iff ->fb).

Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: dri-devel@lists.freedesktop.org
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
 drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 41c38edade74..e1556143d811 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (drm_atomic_plane_disabling(plane, old_plane_state) &&
 		    funcs->atomic_disable)
 			funcs->atomic_disable(plane, old_plane_state);
-		else
+		else if (plane->state->crtc ||
+			 drm_atomic_plane_disabling(plane, old_plane_state))
 			funcs->atomic_update(plane, old_plane_state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 976b89156570..cb383a0fc392 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 	struct intel_plane_state *intel_state =
 		to_intel_plane_state(plane->state);
 
-	/* Don't disable an already disabled plane */
-	if (!plane->state->fb && !old_state->fb)
-		return;
-
 	intel_plane->commit_plane(plane, intel_state);
 }
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6176
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -5              276/276              271/276
ILK                                  301/301              301/301
SNB                 -1              316/316              315/316
IVB                                  328/328              328/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(1)PASS(3)      CRASH(2)
 PNV  igt@gen3_render_linear_blits      FAIL(3)PASS(3)      FAIL(2)
 PNV  igt@gen3_render_mixed_blits      FAIL(2)PASS(3)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(3)PASS(3)      FAIL(2)
 PNV  igt@gen3_render_tiledy_blits      FAIL(3)PASS(3)      FAIL(2)
 SNB  igt@kms_flip@modeset-vs-vblank-race      DMESG_WARN(1)PASS(4)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
drm:intel_dp_start_link_train[i915]]*ERROR*failed_to_get_link_status@failed to get link status
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_start_channel_equalization@failed to start channel equalization
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Fri, 2015-04-10 at 16:22 +0200, Daniel Vetter wrote:
> Since universal planes the primary plane might not be around, and it's
> kinda silly to restrict the pipe bpp to the primary plane if we might
> end up displaying a 10bpc video overlay. And with atomic we might very
> well enable a pipe without a primary plane. So just use the platform
> max as a starting point and then restrict appropriately.
> 
> Of course this is all still a bit moot as long as we artificially
> compress everything to max 8bpc because we don't use the hi-bpc gamma
> tables.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 61 +++++++-----------------------------
>  1 file changed, 12 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 11b958a56515..b1afd7d5e28c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5869,14 +5869,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
>  		return -EINVAL;
>  
> -	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) {
> -		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
> -	} else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8*3) {
> -		/* only a 8bpc pipe, with 6bpc dither through the panel fitter
> -		 * for lvds. */
> -		pipe_config->pipe_bpp = 8*3;
> -	}
> -
>  	if (HAS_IPS(dev))
>  		hsw_compute_ips_config(crtc, pipe_config);
>  
> @@ -10503,7 +10495,6 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>  
>  static int
>  compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> -			  struct drm_framebuffer *fb,
>  			  struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -10511,41 +10502,13 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  	struct intel_connector *connector;
>  	int bpp, i;
>  
> -	switch (fb->pixel_format) {
> -	case DRM_FORMAT_C8:
> -		bpp = 8*3; /* since we go through a colormap */
> -		break;
> -	case DRM_FORMAT_XRGB1555:
> -	case DRM_FORMAT_ARGB1555:
> -		/* checked in intel_framebuffer_init already */
> -		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
> -			return -EINVAL;
> -	case DRM_FORMAT_RGB565:
> -		bpp = 6*3; /* min is 18bpp */
> -		break;
> -	case DRM_FORMAT_XBGR8888:
> -	case DRM_FORMAT_ABGR8888:
> -		/* checked in intel_framebuffer_init already */
> -		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
> -			return -EINVAL;
> -	case DRM_FORMAT_XRGB8888:
> -	case DRM_FORMAT_ARGB8888:
> -		bpp = 8*3;
> -		break;
> -	case DRM_FORMAT_XRGB2101010:
> -	case DRM_FORMAT_ARGB2101010:
> -	case DRM_FORMAT_XBGR2101010:
> -	case DRM_FORMAT_ABGR2101010:
> -		/* checked in intel_framebuffer_init already */
> -		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
> -			return -EINVAL;
> +	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)))
>  		bpp = 10*3;
> -		break;
> -	/* TODO: gen4+ supports 16 bpc floating point, too. */
> -	default:
> -		DRM_DEBUG_KMS("unsupported depth\n");
> -		return -EINVAL;
> -	}
> +	else if (INTEL_INFO(dev)->gen >= 5)
> +		bpp = 12*3;
> +	else
> +		bpp = 8*3;
> +
>  
>  	pipe_config->pipe_bpp = bpp;
>  
> @@ -10756,7 +10719,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	struct intel_crtc_state *pipe_config;
> -	int plane_bpp, ret = -EINVAL;
> +	int base_bpp, ret = -EINVAL;
>  	int i;
>  	bool retry = true;
>  
> @@ -10801,9 +10764,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	 * plane pixel format and any sink constraints into account. Returns the
>  	 * source plane bpp so that dithering can be selected on mismatches
>  	 * after encoders and crtc also have had their say. */
> -	plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
> -					      fb, pipe_config);
> -	if (plane_bpp < 0)
> +	base_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
> +					     pipe_config);
> +	if (base_bpp < 0)
>  		goto fail;
>  
>  	/*
> @@ -10871,9 +10834,9 @@ encoder_retry:
>  		goto encoder_retry;
>  	}
>  
> -	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
> +	pipe_config->dither = pipe_config->pipe_bpp != base_bpp;
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> -		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> +		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
>  	return pipe_config;
>  fail:


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

Re: [PATCH 2/3] drm/i915: Drop unecessary fb arguments from function signatures

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Fri, 2015-04-10 at 16:22 +0200, Daniel Vetter wrote:
> This is a separate patch to simplify conflict handling with other
> ongoing atomic work.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1afd7d5e28c..cdf21df8c69c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10711,7 +10711,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  
>  static struct intel_crtc_state *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
> -			  struct drm_framebuffer *fb,
>  			  struct drm_display_mode *mode,
>  			  struct drm_atomic_state *state)
>  {
> @@ -11514,7 +11513,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  static struct intel_crtc_state *
>  intel_modeset_compute_config(struct drm_crtc *crtc,
>  			     struct drm_display_mode *mode,
> -			     struct drm_framebuffer *fb,
>  			     struct drm_atomic_state *state,
>  			     unsigned *modeset_pipes,
>  			     unsigned *prepare_pipes,
> @@ -11552,7 +11550,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  		if (WARN_ON(&intel_crtc->base != crtc))
>  			continue;
>  
> -		pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
> +		pipe_config = intel_modeset_pipe_config(crtc, mode, state);
>  		if (IS_ERR(pipe_config))
>  			return pipe_config;
>  
> @@ -11758,7 +11756,7 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  	unsigned modeset_pipes, prepare_pipes, disable_pipes;
>  	int ret = 0;
>  
> -	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
> +	pipe_config = intel_modeset_compute_config(crtc, mode, state,
>  						   &modeset_pipes,
>  						   &prepare_pipes,
>  						   &disable_pipes);
> @@ -12216,7 +12214,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		goto fail;
>  
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> -						   set->fb, state,
> +						   state,
>  						   &modeset_pipes,
>  						   &prepare_pipes,
>  						   &disable_pipes);


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

Re: [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Friday 10 April 2015 16:22:39 Daniel Vetter wrote:
> It's a silly thing to do and surprises driver writers. Most likely
> this did already blow up for exynos.
> 
> It's also a silly thing to change plane state when it's off, but fbdev
> is silly (it does an unconditional modeset over all planes). And
> userspace can be evil. So I think we need this.
> 
> With this check in the helpers we can remove the one in i915 code for
> the same conditions (becuase ->crtc iff ->fb).
> 
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(with the ongoing omapdrm atomic update conversion work)

> ---
>  drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) &&
>  		    funcs->atomic_disable)
>  			funcs->atomic_disable(plane, old_plane_state);
> -		else
> +		else if (plane->state->crtc ||
> +			 drm_atomic_plane_disabling(plane, old_plane_state))
>  			funcs->atomic_update(plane, old_plane_state);

The test is so trivial that I wonder whether it makes sense to make 
atomic_disable() optional. Wouldn't it be easier to either make 
atomic_disable() mandatory, or to remove it completely ?

>  	}
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/intel_atomic_plane.c index
> 976b89156570..cb383a0fc392 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane
> *plane, struct intel_plane_state *intel_state =
>  		to_intel_plane_state(plane->state);
> 
> -	/* Don't disable an already disabled plane */
> -	if (!plane->state->fb && !old_state->fb)
> -		return;
> -
>  	intel_plane->commit_plane(plane, intel_state);
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Daniel Vetter]

On Thu, Apr 16, 2015 at 12:46:53AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Friday 10 April 2015 16:22:39 Daniel Vetter wrote:
> > It's a silly thing to do and surprises driver writers. Most likely
> > this did already blow up for exynos.
> > 
> > It's also a silly thing to change plane state when it's off, but fbdev
> > is silly (it does an unconditional modeset over all planes). And
> > userspace can be evil. So I think we need this.
> > 
> > With this check in the helpers we can remove the one in i915 code for
> > the same conditions (becuase ->crtc iff ->fb).
> > 
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> (with the ongoing omapdrm atomic update conversion work)
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> > *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) &&
> >  		    funcs->atomic_disable)
> >  			funcs->atomic_disable(plane, old_plane_state);
> > -		else
> > +		else if (plane->state->crtc ||
> > +			 drm_atomic_plane_disabling(plane, old_plane_state))
> >  			funcs->atomic_update(plane, old_plane_state);
> 
> The test is so trivial that I wonder whether it makes sense to make 
> atomic_disable() optional. Wouldn't it be easier to either make 
> atomic_disable() mandatory, or to remove it completely ?

Yeah it's fairly simple. I think it's more to give drivers some guidance
and to more closely resemble the legacy plane entry points which also had
the split between update and disable. Imo doesn't really hurt to keep this
around.

Merged to topic/drm-misc, thanks for the feedback.
-Daniel

> 
> >  	}
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/intel_atomic_plane.c index
> > 976b89156570..cb383a0fc392 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane
> > *plane, struct intel_plane_state *intel_state =
> >  		to_intel_plane_state(plane->state);
> > 
> > -	/* Don't disable an already disabled plane */
> > -	if (!plane->state->fb && !old_state->fb)
> > -		return;
> > -
> >  	intel_plane->commit_plane(plane, intel_state);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

Re: [PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Paulo Zanoni]

2015-04-15 8:06 GMT-03:00 Ander Conselvan De Oliveira <conselvan2@gmail.com>:
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
>
> On Fri, 2015-04-10 at 16:22 +0200, Daniel Vetter wrote:
>> Since universal planes the primary plane might not be around, and it's
>> kinda silly to restrict the pipe bpp to the primary plane if we might
>> end up displaying a 10bpc video overlay. And with atomic we might very
>> well enable a pipe without a primary plane. So just use the platform
>> max as a starting point and then restrict appropriately.
>>
>> Of course this is all still a bit moot as long as we artificially
>> compress everything to max 8bpc because we don't use the hi-bpc gamma
>> tables.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

This commit makes the HDMI colors look very funny on my BDW machine. I
just boot it with HDMI (no eDP), and I get this:
http://people.freedesktop.org/~pzanoni/color-regression.jpg (the
original wallpaper only has gradients of blue). Although I like the
new wallpaper color scheme, I don't think most users will.

Reverting this commit (and also "drm/i915: Drop unecessary fb
arguments from function signatures" in order to make things compile
again) fixes the problem for me.

Since this is a major issue for me, I'd be happy to test patches.

>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 61 +++++++-----------------------------
>>  1 file changed, 12 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 11b958a56515..b1afd7d5e28c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5869,14 +5869,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>               adjusted_mode->hsync_start == adjusted_mode->hdisplay)
>>               return -EINVAL;
>>
>> -     if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) {
>> -             pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
>> -     } else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8*3) {
>> -             /* only a 8bpc pipe, with 6bpc dither through the panel fitter
>> -              * for lvds. */
>> -             pipe_config->pipe_bpp = 8*3;
>> -     }
>> -
>>       if (HAS_IPS(dev))
>>               hsw_compute_ips_config(crtc, pipe_config);
>>
>> @@ -10503,7 +10495,6 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>>
>>  static int
>>  compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>> -                       struct drm_framebuffer *fb,
>>                         struct intel_crtc_state *pipe_config)
>>  {
>>       struct drm_device *dev = crtc->base.dev;
>> @@ -10511,41 +10502,13 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>>       struct intel_connector *connector;
>>       int bpp, i;
>>
>> -     switch (fb->pixel_format) {
>> -     case DRM_FORMAT_C8:
>> -             bpp = 8*3; /* since we go through a colormap */
>> -             break;
>> -     case DRM_FORMAT_XRGB1555:
>> -     case DRM_FORMAT_ARGB1555:
>> -             /* checked in intel_framebuffer_init already */
>> -             if (WARN_ON(INTEL_INFO(dev)->gen > 3))
>> -                     return -EINVAL;
>> -     case DRM_FORMAT_RGB565:
>> -             bpp = 6*3; /* min is 18bpp */
>> -             break;
>> -     case DRM_FORMAT_XBGR8888:
>> -     case DRM_FORMAT_ABGR8888:
>> -             /* checked in intel_framebuffer_init already */
>> -             if (WARN_ON(INTEL_INFO(dev)->gen < 4))
>> -                     return -EINVAL;
>> -     case DRM_FORMAT_XRGB8888:
>> -     case DRM_FORMAT_ARGB8888:
>> -             bpp = 8*3;
>> -             break;
>> -     case DRM_FORMAT_XRGB2101010:
>> -     case DRM_FORMAT_ARGB2101010:
>> -     case DRM_FORMAT_XBGR2101010:
>> -     case DRM_FORMAT_ABGR2101010:
>> -             /* checked in intel_framebuffer_init already */
>> -             if (WARN_ON(INTEL_INFO(dev)->gen < 4))
>> -                     return -EINVAL;
>> +     if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)))
>>               bpp = 10*3;
>> -             break;
>> -     /* TODO: gen4+ supports 16 bpc floating point, too. */
>> -     default:
>> -             DRM_DEBUG_KMS("unsupported depth\n");
>> -             return -EINVAL;
>> -     }
>> +     else if (INTEL_INFO(dev)->gen >= 5)
>> +             bpp = 12*3;
>> +     else
>> +             bpp = 8*3;
>> +
>>
>>       pipe_config->pipe_bpp = bpp;
>>
>> @@ -10756,7 +10719,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>       struct intel_connector *connector;
>>       struct drm_connector_state *connector_state;
>>       struct intel_crtc_state *pipe_config;
>> -     int plane_bpp, ret = -EINVAL;
>> +     int base_bpp, ret = -EINVAL;
>>       int i;
>>       bool retry = true;
>>
>> @@ -10801,9 +10764,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>        * plane pixel format and any sink constraints into account. Returns the
>>        * source plane bpp so that dithering can be selected on mismatches
>>        * after encoders and crtc also have had their say. */
>> -     plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
>> -                                           fb, pipe_config);
>> -     if (plane_bpp < 0)
>> +     base_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc),
>> +                                          pipe_config);
>> +     if (base_bpp < 0)
>>               goto fail;
>>
>>       /*
>> @@ -10871,9 +10834,9 @@ encoder_retry:
>>               goto encoder_retry;
>>       }
>>
>> -     pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
>> +     pipe_config->dither = pipe_config->pipe_bpp != base_bpp;
>>       DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>> -                   plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>> +                   base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>>
>>       return pipe_config;
>>  fail:
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Daniel Vetter]

On Thu, Apr 30, 2015 at 10:43:39AM -0300, Paulo Zanoni wrote:
> 2015-04-15 8:06 GMT-03:00 Ander Conselvan De Oliveira <conselvan2@gmail.com>:
> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> >
> > On Fri, 2015-04-10 at 16:22 +0200, Daniel Vetter wrote:
> >> Since universal planes the primary plane might not be around, and it's
> >> kinda silly to restrict the pipe bpp to the primary plane if we might
> >> end up displaying a 10bpc video overlay. And with atomic we might very
> >> well enable a pipe without a primary plane. So just use the platform
> >> max as a starting point and then restrict appropriately.
> >>
> >> Of course this is all still a bit moot as long as we artificially
> >> compress everything to max 8bpc because we don't use the hi-bpc gamma
> >> tables.
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> This commit makes the HDMI colors look very funny on my BDW machine. I
> just boot it with HDMI (no eDP), and I get this:
> http://people.freedesktop.org/~pzanoni/color-regression.jpg (the
> original wallpaper only has gradients of blue). Although I like the
> new wallpaper color scheme, I don't think most users will.
> 
> Reverting this commit (and also "drm/i915: Drop unecessary fb
> arguments from function signatures" in order to make things compile
> again) fixes the problem for me.
> 
> Since this is a major issue for me, I'd be happy to test patches.

We dump pipe_bpp and dither settings into dmesg. Do you see any
differences in there between working and broken kernel?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Paulo Zanoni]

2015-05-04 5:34 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Apr 30, 2015 at 10:43:39AM -0300, Paulo Zanoni wrote:
>> 2015-04-15 8:06 GMT-03:00 Ander Conselvan De Oliveira <conselvan2@gmail.com>:
>> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
>> >
>> > On Fri, 2015-04-10 at 16:22 +0200, Daniel Vetter wrote:
>> >> Since universal planes the primary plane might not be around, and it's
>> >> kinda silly to restrict the pipe bpp to the primary plane if we might
>> >> end up displaying a 10bpc video overlay. And with atomic we might very
>> >> well enable a pipe without a primary plane. So just use the platform
>> >> max as a starting point and then restrict appropriately.
>> >>
>> >> Of course this is all still a bit moot as long as we artificially
>> >> compress everything to max 8bpc because we don't use the hi-bpc gamma
>> >> tables.
>> >>
>> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>
>> This commit makes the HDMI colors look very funny on my BDW machine. I
>> just boot it with HDMI (no eDP), and I get this:
>> http://people.freedesktop.org/~pzanoni/color-regression.jpg (the
>> original wallpaper only has gradients of blue). Although I like the
>> new wallpaper color scheme, I don't think most users will.
>>
>> Reverting this commit (and also "drm/i915: Drop unecessary fb
>> arguments from function signatures" in order to make things compile
>> again) fixes the problem for me.
>>
>> Since this is a major issue for me, I'd be happy to test patches.
>
> We dump pipe_bpp and dither settings into dmesg. Do you see any
> differences in there between working and broken kernel?

Just to document the discussion on IRC:

It seems the problem is happening because we're now trying 12bpc
instead of 8bpc, and 12bpc is broken for HDMI. In fact, 12bpc was
already broken before this patch, the problem is that this patch is
now trying to use it. It even generates a WARN, so this bug is
definitely something we can catch with automated systems somehow.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

Re: [PATCH 1/3] drm/i915: Select starting pipe bpp irrespective or the primary plane

[Daniel Vetter]

On Mon, May 4, 2015 at 7:19 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> It seems the problem is happening because we're now trying 12bpc
> instead of 8bpc, and 12bpc is broken for HDMI. In fact, 12bpc was
> already broken before this patch, the problem is that this patch is
> now trying to use it. It even generates a WARN, so this bug is
> definitely something we can catch with automated systems somehow.

Only when we have a 12bpc capable hdmi screen connected. Those kind of
sink-dependant issues are a big gap in igt, Thomas is working on
injecting fake output screens. With that we can fill in a lot of the
missing testcases and hit all these with automated tests.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx