| Message ID | 1428675759-2330-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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:
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
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
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
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
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:
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(-)