[Intel-gfx] [PATCH] sna/uxa: Fix colormap handling at screen depth 30.

[Chris Wilson]

Quoting Ville Syrjälä (2018-03-15 16:02:42)
> On Thu, Mar 15, 2018 at 03:28:18PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-03-01 11:12:53)
> > > On Thu, Mar 01, 2018 at 02:20:48AM +0100, Mario Kleiner wrote:
> > > > The various clut handling functions like a setup
> > > > consistent with the x-screen color depth. Otherwise
> > > > we observe improper sampling in the gamma tables
> > > > at depth 30.
> > > > 
> > > > Therefore replace hard-coded bitsPerRGB = 8 by actual
> > > > bits per channel scrn->rgbBits. Also use this for call
> > > > to xf86HandleColormaps().
> > > > 
> > > > Tested for uxa and sna at depths 8, 16, 24 and 30 on
> > > > IvyBridge, and tested at depth 24 and 30 that xgamma
> > > > and gamma table animations work, and with measurement
> > > > equipment to make sure identity gamma ramps actually
> > > > are identity mappings at the output.
> > > 
> > > You mean identity mapping at 8bpc? We don't support higher precision
> > > gamma on pre-bdw atm, and the ddx doesn't use the higher precision
> > > stuff even on bdw+. I'm working on fixing both, but it turned out to
> > > be a bit more work than I anticipated so will take a while.
> > > 
> > > > 
> > > > Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> > > > ---
> > > >  src/sna/sna_driver.c   | 5 +++--
> > > >  src/uxa/intel_driver.c | 3 ++-
> > > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
> > > > index 2643e6c..9c4bcd4 100644
> > > > --- a/src/sna/sna_driver.c
> > > > +++ b/src/sna/sna_driver.c
> > > > @@ -1145,7 +1145,7 @@ sna_screen_init(SCREEN_INIT_ARGS_DECL)
> > > >       if (!miInitVisuals(&visuals, &depths, &nvisuals, &ndepths, &rootdepth,
> > > >                          &defaultVisual,
> > > >                          ((unsigned long)1 << (scrn->bitsPerPixel - 1)),
> > > > -                        8, -1))
> > > > +                        scrn->rgbBits, -1))
> > > >               return FALSE;
> > > >  
> > > >       if (!miScreenInit(screen, NULL,
> > > > @@ -1217,7 +1217,8 @@ sna_screen_init(SCREEN_INIT_ARGS_DECL)
> > > >               return FALSE;
> > > >  
> > > >       if (sna->mode.num_real_crtc &&
> > > > -         !xf86HandleColormaps(screen, 256, 8, sna_load_palette, NULL,
> > > > +         !xf86HandleColormaps(screen, 1 << scrn->rgbBits, scrn->rgbBits,
> > > > +                              sna_load_palette, NULL,
> > > >                                CMAP_RELOAD_ON_MODE_SWITCH |
> > > >                                CMAP_PALETTED_TRUECOLOR))
> > > 
> > > I already forgot what this does prior to your randr fix. IIRC bumping
> > > the 8 alone would cause the thing to segfault, but I guess bumping both
> > > was fine?
> > > 
> > > Hmm. So the server always initializes crtc->gamma_size to 256
> > > (which does match the normal hw LUT size), and so before your
> > > fix we will always get gamma_slots==0 at >8bpc and so the hw LUT
> > > is never actually updated?
> > 
> > Was there any conclusion to this? Aiui, these bits should be set based
> > on the underlying HW gamma table depth which is not the same as the
> > screen colour depth. It stuck at 256 for ages as that is all anyone
> > ever expected...
> 
> IIRC there was some kind of agreement that Mario's approach here is
> generally what we want to do. The server randr code will simply throw
> away the LUT entries we can't use and driver will still get the
> expected 256 entry LUT via the randr hooks. Obviously that means the
> output may not be exactly what the user wanted, but for normal
> gamma curve type of stuff it's pretty close.
> 
> I'm mostly concerned what happens when the server doesn't have
> Mario's gamma fixes. IIRC something will either crash, or simply
> not update the gamma LUT ever.

I guess we just make it conditional on xorg_version >= 1.20 to be on the
safe side.

> Once I manage to pimp up the i915 gamma LUT stuff I plan to tackle
> the ddx side to actually make use of the higher precision gamma modes
> in the hardware. Actually I already started on the ddx side a bit just
> didn't get the kernel quite ready yet. Once it's all done we should 
> have 10bit gamma to match the screen depth on all platforms. On gen4
> it will be an interpolated curve though, so if the user programs an
> arbitrary LUT we'll not be able to replicate it correctly. But again,
> for normal gamma curve type of stuff it'll work just fine. For ilk+
> we will get a full 1024 entry LUT.

Sounds good.
-Chris