| Message ID | 1460998073-12105-1-git-send-email-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 18-04-16 om 18:47 schreef Bob Paauwe: > The i915 driver is now using atomic properties and atomic commit > to handle the legacy set gamma IOCTL. However, if the driver is > configured without atomic (nuclear_pageflip = false), it won't > update the legacy properties for degamma_lut, gamma_lut and ctm > leaving them out of sync with the atomic version of the properties. > > Until the driver is full atomic, make sure we update the non-atomic > version of the properties. > > v2: Update the comment with a FIXME. (Daniel) > Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? Not sure why this patch is required on top.
Op 18-04-16 om 18:47 schreef Bob Paauwe: > The i915 driver is now using atomic properties and atomic commit > to handle the legacy set gamma IOCTL. However, if the driver is > configured without atomic (nuclear_pageflip = false), it won't > update the legacy properties for degamma_lut, gamma_lut and ctm > leaving them out of sync with the atomic version of the properties. > > Until the driver is full atomic, make sure we update the non-atomic > version of the properties. > > v2: Update the comment with a FIXME. (Daniel) > > igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5155efb6..a97bbff 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13885,8 +13885,45 @@ out: > > #undef for_each_intel_crtc_masked > > +/* > + * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling > + * drm_atomic_helper_legacy_gamma_set() directly. > + */ > +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc, > + u16 *red, u16 *green, u16 *blue, > + uint32_t start, uint32_t size) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_crtc_state *state; > + > + drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size); > On a side note, the gamma_set function pointer should really be an int, because it may fail with -EINTR. ~Maarten
On 19/04/16 07:02, Maarten Lankhorst wrote: > Op 18-04-16 om 18:47 schreef Bob Paauwe: >> The i915 driver is now using atomic properties and atomic commit >> to handle the legacy set gamma IOCTL. However, if the driver is >> configured without atomic (nuclear_pageflip = false), it won't >> update the legacy properties for degamma_lut, gamma_lut and ctm >> leaving them out of sync with the atomic version of the properties. >> >> Until the driver is full atomic, make sure we update the non-atomic >> version of the properties. >> >> v2: Update the comment with a FIXME. (Daniel) >> > Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? > Not sure why this patch is required on top. > Daniel would prefer to have this fix only in the i915 driver as this is the last driver still transitioning to atomic.
On Tue, 19 Apr 2016 08:05:26 +0200 Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 18-04-16 om 18:47 schreef Bob Paauwe: > > The i915 driver is now using atomic properties and atomic commit > > to handle the legacy set gamma IOCTL. However, if the driver is > > configured without atomic (nuclear_pageflip = false), it won't > > update the legacy properties for degamma_lut, gamma_lut and ctm > > leaving them out of sync with the atomic version of the properties. > > > > Until the driver is full atomic, make sure we update the non-atomic > > version of the properties. > > > > v2: Update the comment with a FIXME. (Daniel) > > > > igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 5155efb6..a97bbff 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13885,8 +13885,45 @@ out: > > > > #undef for_each_intel_crtc_masked > > > > +/* > > + * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling > > + * drm_atomic_helper_legacy_gamma_set() directly. > > + */ > > +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc, > > + u16 *red, u16 *green, u16 *blue, > > + uint32_t start, uint32_t size) > > +{ > > + struct drm_device *dev = crtc->dev; > > + struct drm_mode_config *config = &dev->mode_config; > > + struct drm_crtc_state *state; > > + > > + drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size); > > > On a side note, the gamma_set function pointer should really be an int, because it may fail with -EINTR. ??? from drm_crtc.h void (*gamma_set)(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, uint32_t start, uint32_t size); Or are you saying that the gamma_set should be capable of returning an error that can be propagated up through the IOCTL? Bob > > ~Maarten
Op 19-04-16 om 18:20 schreef Bob Paauwe: > On Tue, 19 Apr 2016 08:05:26 +0200 > Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > >> Op 18-04-16 om 18:47 schreef Bob Paauwe: >>> The i915 driver is now using atomic properties and atomic commit >>> to handle the legacy set gamma IOCTL. However, if the driver is >>> configured without atomic (nuclear_pageflip = false), it won't >>> update the legacy properties for degamma_lut, gamma_lut and ctm >>> leaving them out of sync with the atomic version of the properties. >>> >>> Until the driver is full atomic, make sure we update the non-atomic >>> version of the properties. >>> >>> v2: Update the comment with a FIXME. (Daniel) >>> >>> igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++- >>> 1 file changed, 38 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 5155efb6..a97bbff 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -13885,8 +13885,45 @@ out: >>> >>> #undef for_each_intel_crtc_masked >>> >>> +/* >>> + * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling >>> + * drm_atomic_helper_legacy_gamma_set() directly. >>> + */ >>> +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc, >>> + u16 *red, u16 *green, u16 *blue, >>> + uint32_t start, uint32_t size) >>> +{ >>> + struct drm_device *dev = crtc->dev; >>> + struct drm_mode_config *config = &dev->mode_config; >>> + struct drm_crtc_state *state; >>> + >>> + drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size); >>> >> On a side note, the gamma_set function pointer should really be an int, because it may fail with -EINTR. > ??? from drm_crtc.h > > void (*gamma_set)(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, > uint32_t start, uint32_t size); > > Or are you saying that the gamma_set should be capable of returning an > error that can be propagated up through the IOCTL? > Yes, because it may fail with -EINTR. ~Maarten
Op 19-04-16 om 12:13 schreef Lionel Landwerlin: > On 19/04/16 07:02, Maarten Lankhorst wrote: >> Op 18-04-16 om 18:47 schreef Bob Paauwe: >>> The i915 driver is now using atomic properties and atomic commit >>> to handle the legacy set gamma IOCTL. However, if the driver is >>> configured without atomic (nuclear_pageflip = false), it won't >>> update the legacy properties for degamma_lut, gamma_lut and ctm >>> leaving them out of sync with the atomic version of the properties. >>> >>> Until the driver is full atomic, make sure we update the non-atomic >>> version of the properties. >>> >>> v2: Update the comment with a FIXME. (Daniel) >>> >> Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? >> Not sure why this patch is required on top. >> > Daniel would prefer to have this fix only in the i915 driver as this is the last driver still transitioning to atomic. > Oh indeed, lack of DRIVER_ATOMIC cap again. :( Still feels like something that doesn't belong in the driver, but ok.. ~Maarten
On Wed, Apr 20, 2016 at 09:00:09AM +0200, Maarten Lankhorst wrote: > Op 19-04-16 om 12:13 schreef Lionel Landwerlin: > > On 19/04/16 07:02, Maarten Lankhorst wrote: > >> Op 18-04-16 om 18:47 schreef Bob Paauwe: > >>> The i915 driver is now using atomic properties and atomic commit > >>> to handle the legacy set gamma IOCTL. However, if the driver is > >>> configured without atomic (nuclear_pageflip = false), it won't > >>> update the legacy properties for degamma_lut, gamma_lut and ctm > >>> leaving them out of sync with the atomic version of the properties. > >>> > >>> Until the driver is full atomic, make sure we update the non-atomic > >>> version of the properties. > >>> > >>> v2: Update the comment with a FIXME. (Daniel) > >>> > >> Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? > >> Not sure why this patch is required on top. > >> > > Daniel would prefer to have this fix only in the i915 driver as this is the last driver still transitioning to atomic. > > > Oh indeed, lack of DRIVER_ATOMIC cap again. :( > Still feels like something that doesn't belong in the driver, but ok.. Agreed, hence why I asked for a FIXME comment so we don't forget to rip this out once we enale DRIVER_ATOMIC for real. -Daniel
On 20/04/16 13:38, Daniel Vetter wrote: > On Wed, Apr 20, 2016 at 09:00:09AM +0200, Maarten Lankhorst wrote: >> Op 19-04-16 om 12:13 schreef Lionel Landwerlin: >>> On 19/04/16 07:02, Maarten Lankhorst wrote: >>>> Op 18-04-16 om 18:47 schreef Bob Paauwe: >>>>> The i915 driver is now using atomic properties and atomic commit >>>>> to handle the legacy set gamma IOCTL. However, if the driver is >>>>> configured without atomic (nuclear_pageflip = false), it won't >>>>> update the legacy properties for degamma_lut, gamma_lut and ctm >>>>> leaving them out of sync with the atomic version of the properties. >>>>> >>>>> Until the driver is full atomic, make sure we update the non-atomic >>>>> version of the properties. >>>>> >>>>> v2: Update the comment with a FIXME. (Daniel) >>>>> >>>> Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? >>>> Not sure why this patch is required on top. >>>> >>> Daniel would prefer to have this fix only in the i915 driver as this is the last driver still transitioning to atomic. >>> >> Oh indeed, lack of DRIVER_ATOMIC cap again. :( >> Still feels like something that doesn't belong in the driver, but ok.. > Agreed, hence why I asked for a FIXME comment so we don't forget to rip > this out once we enale DRIVER_ATOMIC for real. > -Daniel Hi Daniel, Was there anything missing in the last version of this patch? Thanks, - Lionel
On Wed, May 04, 2016 at 02:49:50PM +0100, Lionel Landwerlin wrote: > On 20/04/16 13:38, Daniel Vetter wrote: > >On Wed, Apr 20, 2016 at 09:00:09AM +0200, Maarten Lankhorst wrote: > >>Op 19-04-16 om 12:13 schreef Lionel Landwerlin: > >>>On 19/04/16 07:02, Maarten Lankhorst wrote: > >>>>Op 18-04-16 om 18:47 schreef Bob Paauwe: > >>>>>The i915 driver is now using atomic properties and atomic commit > >>>>>to handle the legacy set gamma IOCTL. However, if the driver is > >>>>>configured without atomic (nuclear_pageflip = false), it won't > >>>>>update the legacy properties for degamma_lut, gamma_lut and ctm > >>>>>leaving them out of sync with the atomic version of the properties. > >>>>> > >>>>>Until the driver is full atomic, make sure we update the non-atomic > >>>>>version of the properties. > >>>>> > >>>>>v2: Update the comment with a FIXME. (Daniel) > >>>>> > >>>>Seems to me that this is fixed by [PATCH] drm: atomic: fix legacy gamma set helper? > >>>>Not sure why this patch is required on top. > >>>> > >>>Daniel would prefer to have this fix only in the i915 driver as this is the last driver still transitioning to atomic. > >>> > >>Oh indeed, lack of DRIVER_ATOMIC cap again. :( > >>Still feels like something that doesn't belong in the driver, but ok.. > >Agreed, hence why I asked for a FIXME comment so we don't forget to rip > >this out once we enale DRIVER_ATOMIC for real. > >-Daniel > Hi Daniel, > > Was there anything missing in the last version of this patch? Pending review comments from Maarten, and no r-b tag. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5155efb6..a97bbff 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13885,8 +13885,45 @@ out: #undef for_each_intel_crtc_masked +/* + * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling + * drm_atomic_helper_legacy_gamma_set() directly. + */ +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc, + u16 *red, u16 *green, u16 *blue, + uint32_t start, uint32_t size) +{ + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + struct drm_crtc_state *state; + + drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size); + + /* + * Make sure we update the legacy properties so this works when + * atomic is not enabled. + */ + + state = crtc->state; + + drm_object_property_set_value(&crtc->base, + config->degamma_lut_property, + (state->degamma_lut) ? + state->degamma_lut->base.id : 0); + + drm_object_property_set_value(&crtc->base, + config->ctm_property, + (state->ctm) ? + state->ctm->base.id : 0); + + drm_object_property_set_value(&crtc->base, + config->gamma_lut_property, + (state->gamma_lut) ? + state->gamma_lut->base.id : 0); +} + static const struct drm_crtc_funcs intel_crtc_funcs = { - .gamma_set = drm_atomic_helper_legacy_gamma_set, + .gamma_set = intel_atomic_legacy_gamma_set, .set_config = drm_atomic_helper_set_config, .set_property = drm_atomic_helper_crtc_set_property, .destroy = intel_crtc_destroy,
The i915 driver is now using atomic properties and atomic commit to handle the legacy set gamma IOCTL. However, if the driver is configured without atomic (nuclear_pageflip = false), it won't update the legacy properties for degamma_lut, gamma_lut and ctm leaving them out of sync with the atomic version of the properties. Until the driver is full atomic, make sure we update the non-atomic version of the properties. v2: Update the comment with a FIXME. (Daniel) igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)