diff mbox

drm/i915: enable IPS for bpp <= 24

Message ID 1374772010-3115-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes July 25, 2013, 5:06 p.m. UTC
Art confirms that this should work fine.  Since most panels are 18bpp
with dithering from 24bpp, the existing code wouldn't be enabled in most
cases.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chris Wilson July 25, 2013, 5:17 p.m. UTC | #1
On Thu, Jul 25, 2013 at 10:06:50AM -0700, Jesse Barnes wrote:
> Art confirms that this should work fine.  Since most panels are 18bpp
> with dithering from 24bpp, the existing code wouldn't be enabled in most
> cases.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 12ea1a9..7cf475b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4091,7 +4091,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>  {
>  	pipe_config->ips_enabled = i915_enable_ips &&
>  				   hsw_crtc_supports_ips(crtc) &&
> -				   pipe_config->pipe_bpp == 24;
> +				   pipe_config->pipe_bpp <= 24;

Don't you want to check that the incoming fb is depth 24?
-Chris
Jesse Barnes July 25, 2013, 5:27 p.m. UTC | #2
On Thu, 25 Jul 2013 18:17:59 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, Jul 25, 2013 at 10:06:50AM -0700, Jesse Barnes wrote:
> > Art confirms that this should work fine.  Since most panels are 18bpp
> > with dithering from 24bpp, the existing code wouldn't be enabled in most
> > cases.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 12ea1a9..7cf475b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4091,7 +4091,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
> >  {
> >  	pipe_config->ips_enabled = i915_enable_ips &&
> >  				   hsw_crtc_supports_ips(crtc) &&
> > -				   pipe_config->pipe_bpp == 24;
> > +				   pipe_config->pipe_bpp <= 24;
> 
> Don't you want to check that the incoming fb is depth 24?

I think this field will have the right bits in it for a 12bpc HDMI
config for example, so we should be able to use it for IPS support.
Daniel Vetter July 25, 2013, 5:38 p.m. UTC | #3
On Thu, Jul 25, 2013 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 25 Jul 2013 18:17:59 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
>> On Thu, Jul 25, 2013 at 10:06:50AM -0700, Jesse Barnes wrote:
>> > Art confirms that this should work fine.  Since most panels are 18bpp
>> > with dithering from 24bpp, the existing code wouldn't be enabled in most
>> > cases.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 12ea1a9..7cf475b 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -4091,7 +4091,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>> >  {
>> >     pipe_config->ips_enabled = i915_enable_ips &&
>> >                                hsw_crtc_supports_ips(crtc) &&
>> > -                              pipe_config->pipe_bpp == 24;
>> > +                              pipe_config->pipe_bpp <= 24;
>>
>> Don't you want to check that the incoming fb is depth 24?
>
> I think this field will have the right bits in it for a 12bpc HDMI
> config for example, so we should be able to use it for IPS support.

Atm the above should work, but I expect that longer-term we need to
track sprite/plane configuration bits (like the watermark stuff
maybe), too. Only HDMI rounds up from the fb bpp value, but it doesn't
round up 8bpc to 12bpc. This might change eventually when we get
around to implement the high-precision gamma tables, since with a
10bpc display it would make sense to have the higher precision even
with a 24bpp scanout buffer.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Paulo Zanoni July 31, 2013, 8:35 p.m. UTC | #4
2013/7/25 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Art confirms that this should work fine.  Since most panels are 18bpp
> with dithering from 24bpp, the existing code wouldn't be enabled in most
> cases.
>

I previously said I tested this in the past and it didn't work. So
today I tested it again and I discovered that the problem is that if I
configure X to use DefaultDepth 16, my eDP monitor won't work *at
all*, even with IPS disabled. Everything else seems to work.

Sorry for the wrong information.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 12ea1a9..7cf475b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4091,7 +4091,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>  {
>         pipe_config->ips_enabled = i915_enable_ips &&
>                                    hsw_crtc_supports_ips(crtc) &&
> -                                  pipe_config->pipe_bpp == 24;
> +                                  pipe_config->pipe_bpp <= 24;
>  }
>
>  static int intel_crtc_compute_config(struct intel_crtc *crtc,
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Aug. 5, 2013, 5:32 a.m. UTC | #5
On Wed, Jul 31, 2013 at 05:35:53PM -0300, Paulo Zanoni wrote:
> 2013/7/25 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > Art confirms that this should work fine.  Since most panels are 18bpp
> > with dithering from 24bpp, the existing code wouldn't be enabled in most
> > cases.
> >
> 
> I previously said I tested this in the past and it didn't work. So
> today I tested it again and I discovered that the problem is that if I
> configure X to use DefaultDepth 16, my eDP monitor won't work *at
> all*, even with IPS disabled. Everything else seems to work.
> 
> Sorry for the wrong information.

I guess this is another case of

https://bugzilla.kernel.org/show_bug.cgi?id=59841

Care to test the attached patch there?

> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged, thanks for the patch&review.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12ea1a9..7cf475b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4091,7 +4091,7 @@  static void hsw_compute_ips_config(struct intel_crtc *crtc,
 {
 	pipe_config->ips_enabled = i915_enable_ips &&
 				   hsw_crtc_supports_ips(crtc) &&
-				   pipe_config->pipe_bpp == 24;
+				   pipe_config->pipe_bpp <= 24;
 }
 
 static int intel_crtc_compute_config(struct intel_crtc *crtc,