Re: [PATCH] drm/i915: Fix legacy gamma lut updates in Linux 4.7-rc6

[Mario Kleiner]

On 07/12/2016 05:02 PM, Lionel Landwerlin wrote:
> On 12/07/16 13:11, Mario Kleiner wrote:
> > On 07/12/2016 12:50 PM, Lionel Landwerlin wrote:
> > > Hi Mario,
> >
> > Hi Lionel,
> >
> > > There was a couple of patch to fix this issue :
> > >
> > > https://patchwork.freedesktop.org/series/5467/
> > > https://patchwork.freedesktop.org/series/5466/
> >
> > Looking at them they should fix the issue, but they seem to be stuck
> > in review?
> >
> > > I tested this late last week on drm-intel-nightly, it seems a series of
> > > revert fixed most of the issues.
> >
> > You mean something else has fixed legacy gamma updates, as i can't
> > find above patches applied on drm-intel-nightly?
>
> This revert on drm-intel-nightly seems to have fixed the problem :
>
> https://cgit.freedesktop.org/drm-intel/commit/drivers/gpu/drm/i915?id=e42aeef1237b7c969a77b7f726c50f6cb832185f

Ok, with that intel-nightly looks like drm-next for 4.8 and that indeed 
has working lut updates in my testing. My own patch was motivated by the 
way the implementation is done in intel_atomic_commit_tail() from drm-next.

> > Are those fixes supposed to be already part of 4.7-rc7, the final rc
> > afaik?
>
> I haven't seen it on 4.7-rc7.

I just checked Linus tree for 4.7-rc7 and there the code in 
intel_display.c didn't receive any updates since 13 days and looks like 
the broken code from rc6 which according to my testing doesn't work.

So i'd assume legacy gamma table updates are broken in Linux 4.7 final 
rc atm. Couldn't test, because for some weird reason 4.7-rc7 doesn't 
even boot on my laptop :( - However i got that via a quick install from 
Ubuntu's mainline ppa so it could be some unrelated problem with their 
ppa builds.

I think either my patch would fix it, but is untested wrt. nuclear 
pageflip, or those two patches you referenced, which apparently didn't 
move forward.

What now?
-mario

> > thanks,
> > -mario
> >
> >
> > > Cheers,
> > >
> > > -
> > > Lionel
> > >
> > > On 12/07/16 11:33, Mario Kleiner wrote:
> > > > Updating legacy gamma tables, e.g., via RandR doesn't work at all
> > > > as of Linux 4.7-rc6.
> > > >
> > > > Reason seems to be that the required call to
> > > > drm_atomic_helper_commit_planes_on_crtc is skipped in
> > > > intel_atomic_commit after userspace set new gamma tables,
> > > > because neither crtc->state->planes_changed nor
> > > > update_pipe (= pipe_config->update_pipe) are true.
> > > >
> > > > Removing the check for planes_changed || update_pipe fixes
> > > > gamma table updates.
> > > >
> > > > The code for Linux 4.8 drm-next has changed a lot in that area
> > > > wrt. 4.7, but the new code for 4.8 also removed those checks
> > > > and calls drm_atomic_helper_commit_planes_on_crtc unconditionally,
> > > > and legacy gamma lut updates work on drm-next, so this seems to be
> > > > the right solution.
> > > >
> > > > Tested also shutdown/reboot, suspend/resume, (un-)plugging displays,
> > > > mode switches for resolution/refresh rate, display rotation, and
> > > > page-flipping/pageflip timing on Intel HD Ironlake to confirm the
> > > > fix apparently doesn't break anything under X11.
> > > >
> > > > Signed-off-by: Mario Kleiner <mario.kleiner.de@xxxxxxxxx>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > > Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_display.c | 4 +---
> > > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 04452cf..eb8fb36 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -13685,7 +13685,6 @@ static int intel_atomic_commit(struct
> > > > drm_device *dev,
> > > >           bool modeset = needs_modeset(crtc->state);
> > > >           struct intel_crtc_state *pipe_config =
> > > >               to_intel_crtc_state(crtc->state);
> > > > -        bool update_pipe = !modeset && pipe_config->update_pipe;
> > > >           if (modeset && crtc->state->active) {
> > > >               update_scanline_offset(to_intel_crtc(crtc));
> > > > @@ -13699,8 +13698,7 @@ static int intel_atomic_commit(struct
> > > > drm_device *dev,
> > > >               drm_atomic_get_existing_plane_state(state,
> > > > crtc->primary))
> > > >               intel_fbc_enable(intel_crtc);
> > > > -        if (crtc->state->active &&
> > > > -            (crtc->state->planes_changed || update_pipe))
> > > > +        if (crtc->state->active)
> > > > drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> > > >           if (pipe_config->base.active &&
> > > > needs_vblank_wait(pipe_config))