Re: [PATCH] sna/uxa: Fix colormap handling at screen depth 30.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx