| Message ID | 1555064463-18479-7-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Multi Segment Gamma Support | expand |
fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > Introduced a client cap for advance cap mode > capability. Userspace should set this to get > to be able to use the new gamma_mode property. > > If this is not set, driver will work in legacy > mode. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> Nack, this doesn't seem like a sensible idea. We already guard it behind the gamma mode property. Userspace shouldn't set the gamma mode to a value it doesn't understand. ~Maarten > drivers/gpu/drm/drm_atomic_uapi.c | 3 +++ > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > include/drm/drm_atomic.h | 1 + > include/drm/drm_crtc.h | 7 +++++++ > include/drm/drm_file.h | 8 ++++++++ > include/uapi/drm/drm.h | 2 ++ > 6 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index d85e0c9..590c87a 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct > drm_atomic_state *state, > break; > } > > + crtc_state->advance_gamma_mode_active = > + state- > >advance_gamma_mode_active; > ret = drm_atomic_crtc_set_property(crtc, > crtc_state, prop, prop_value); > break; > @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device > *dev, > drm_modeset_acquire_init(&ctx, > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > state->acquire_ctx = &ctx; > state->allow_modeset = !!(arg->flags & > DRM_MODE_ATOMIC_ALLOW_MODESET); > + state->advance_gamma_mode_active = file_priv- > >advance_gamma_mode_active; > > retry: > copied_objs = 0; > diff --git a/drivers/gpu/drm/drm_ioctl.c > b/drivers/gpu/drm/drm_ioctl.c > index d337f16..e593a4c 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev, > void *data, struct drm_file *file_ > return -EINVAL; > file_priv->writeback_connectors = req->value; > break; > + case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES: > + if (req->value > 1) > + return -EINVAL; > + file_priv->advance_gamma_mode_active = req->value; > + break; > default: > return -EINVAL; > } > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 824a5ed..02c1a68 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -338,6 +338,7 @@ struct drm_atomic_state { > * states. > */ > bool duplicated : 1; > + bool advance_gamma_mode_active : 1; > struct __drm_planes_state *planes; > struct __drm_crtcs_state *crtcs; > int num_connector; > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f789359..f11dc25 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -170,6 +170,11 @@ struct drm_crtc_state { > bool color_mgmt_changed : 1; > > /** > + * This is to indicate advance gamma mode support > + */ > + bool advance_gamma_mode_active : 1; > + > + /** > * @no_vblank: > * > * Reflects the ability of a CRTC to send VBLANK events. > This state > @@ -952,6 +957,8 @@ struct drm_crtc { > */ > bool enabled; > > + bool advance_gamma_mode_active : 1; > + > /** > * @mode: > * > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 6710b61..b5aa24e 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -201,6 +201,14 @@ struct drm_file { > bool writeback_connectors; > > /** > + * This is to enable advance gamma modes using > + * gamma_mode property > + * > + * True if client understands advance gamma > + */ > + bool advance_gamma_mode_active : 1; > + > + /** > * @is_master: > * > * This client is the creator of @master. Protected by > struct > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 236b01a..abef966 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -696,6 +696,8 @@ struct drm_get_cap { > */ > #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5 > > +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES 6 > + > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > struct drm_set_client_cap { > __u64 capability;
On Mon, Apr 15, 2019 at 10:57:52AM +0000, Lankhorst, Maarten wrote: > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > Introduced a client cap for advance cap mode > > capability. Userspace should set this to get > > to be able to use the new gamma_mode property. > > > > If this is not set, driver will work in legacy > > mode. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > Nack, this doesn't seem like a sensible idea. We already guard it > behind the gamma mode property. Userspace shouldn't set the gamma mode > to a value it doesn't understand. Old userspace wouldn't know what a gamma_mode prop is. While a client cap isn't an entirely satisfactory solution I can't think of a better way at the moment. Well, maybe the old "hey kernel, please reset all my props to some sane default" idea could be another way to deal with this sort of stuff. > > ~Maarten > > > drivers/gpu/drm/drm_atomic_uapi.c | 3 +++ > > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > > include/drm/drm_atomic.h | 1 + > > include/drm/drm_crtc.h | 7 +++++++ > > include/drm/drm_file.h | 8 ++++++++ > > include/uapi/drm/drm.h | 2 ++ > > 6 files changed, 26 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > > b/drivers/gpu/drm/drm_atomic_uapi.c > > index d85e0c9..590c87a 100644 > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct > > drm_atomic_state *state, > > break; > > } > > > > + crtc_state->advance_gamma_mode_active = > > + state- > > >advance_gamma_mode_active; > > ret = drm_atomic_crtc_set_property(crtc, > > crtc_state, prop, prop_value); > > break; > > @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device > > *dev, > > drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > state->acquire_ctx = &ctx; > > state->allow_modeset = !!(arg->flags & > > DRM_MODE_ATOMIC_ALLOW_MODESET); > > + state->advance_gamma_mode_active = file_priv- > > >advance_gamma_mode_active; > > > > retry: > > copied_objs = 0; > > diff --git a/drivers/gpu/drm/drm_ioctl.c > > b/drivers/gpu/drm/drm_ioctl.c > > index d337f16..e593a4c 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev, > > void *data, struct drm_file *file_ > > return -EINVAL; > > file_priv->writeback_connectors = req->value; > > break; > > + case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES: > > + if (req->value > 1) > > + return -EINVAL; > > + file_priv->advance_gamma_mode_active = req->value; > > + break; > > default: > > return -EINVAL; > > } > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index 824a5ed..02c1a68 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -338,6 +338,7 @@ struct drm_atomic_state { > > * states. > > */ > > bool duplicated : 1; > > + bool advance_gamma_mode_active : 1; > > struct __drm_planes_state *planes; > > struct __drm_crtcs_state *crtcs; > > int num_connector; > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index f789359..f11dc25 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -170,6 +170,11 @@ struct drm_crtc_state { > > bool color_mgmt_changed : 1; > > > > /** > > + * This is to indicate advance gamma mode support > > + */ > > + bool advance_gamma_mode_active : 1; > > + > > + /** > > * @no_vblank: > > * > > * Reflects the ability of a CRTC to send VBLANK events. > > This state > > @@ -952,6 +957,8 @@ struct drm_crtc { > > */ > > bool enabled; > > > > + bool advance_gamma_mode_active : 1; > > + > > /** > > * @mode: > > * > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > > index 6710b61..b5aa24e 100644 > > --- a/include/drm/drm_file.h > > +++ b/include/drm/drm_file.h > > @@ -201,6 +201,14 @@ struct drm_file { > > bool writeback_connectors; > > > > /** > > + * This is to enable advance gamma modes using > > + * gamma_mode property > > + * > > + * True if client understands advance gamma > > + */ > > + bool advance_gamma_mode_active : 1; > > + > > + /** > > * @is_master: > > * > > * This client is the creator of @master. Protected by > > struct > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 236b01a..abef966 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -696,6 +696,8 @@ struct drm_get_cap { > > */ > > #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5 > > > > +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES 6 > > + > > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > > struct drm_set_client_cap { > > __u64 capability; > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Lankhorst, Maarten > Sent: Monday, April 15, 2019 4:28 PM > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri- > devel@lists.freedesktop.org > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.com; > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>; > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.org; > Sharma, Shashank <shashank.sharma@intel.com> > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > Introduced a client cap for advance cap mode > > capability. Userspace should set this to get > > to be able to use the new gamma_mode property. > > > > If this is not set, driver will work in legacy > > mode. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > Nack, this doesn't seem like a sensible idea. We already guard it > behind the gamma mode property. Userspace shouldn't set the gamma mode > to a value it doesn't understand. > > ~Maarten Hey Maarten, In that case, what do you suggest should be the right way to do this ? @Ville, any comments here ? Regards Shashank > > > drivers/gpu/drm/drm_atomic_uapi.c | 3 +++ > > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > > include/drm/drm_atomic.h | 1 + > > include/drm/drm_crtc.h | 7 +++++++ > > include/drm/drm_file.h | 8 ++++++++ > > include/uapi/drm/drm.h | 2 ++ > > 6 files changed, 26 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > > b/drivers/gpu/drm/drm_atomic_uapi.c > > index d85e0c9..590c87a 100644 > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct > > drm_atomic_state *state, > > break; > > } > > > > + crtc_state->advance_gamma_mode_active = > > + state- > > >advance_gamma_mode_active; > > ret = drm_atomic_crtc_set_property(crtc, > > crtc_state, prop, prop_value); > > break; > > @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device > > *dev, > > drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > state->acquire_ctx = &ctx; > > state->allow_modeset = !!(arg->flags & > > DRM_MODE_ATOMIC_ALLOW_MODESET); > > + state->advance_gamma_mode_active = file_priv- > > >advance_gamma_mode_active; > > > > retry: > > copied_objs = 0; > > diff --git a/drivers/gpu/drm/drm_ioctl.c > > b/drivers/gpu/drm/drm_ioctl.c > > index d337f16..e593a4c 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev, > > void *data, struct drm_file *file_ > > return -EINVAL; > > file_priv->writeback_connectors = req->value; > > break; > > + case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES: > > + if (req->value > 1) > > + return -EINVAL; > > + file_priv->advance_gamma_mode_active = req->value; > > + break; > > default: > > return -EINVAL; > > } > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index 824a5ed..02c1a68 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -338,6 +338,7 @@ struct drm_atomic_state { > > * states. > > */ > > bool duplicated : 1; > > + bool advance_gamma_mode_active : 1; > > struct __drm_planes_state *planes; > > struct __drm_crtcs_state *crtcs; > > int num_connector; > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index f789359..f11dc25 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -170,6 +170,11 @@ struct drm_crtc_state { > > bool color_mgmt_changed : 1; > > > > /** > > + * This is to indicate advance gamma mode support > > + */ > > + bool advance_gamma_mode_active : 1; > > + > > + /** > > * @no_vblank: > > * > > * Reflects the ability of a CRTC to send VBLANK events. > > This state > > @@ -952,6 +957,8 @@ struct drm_crtc { > > */ > > bool enabled; > > > > + bool advance_gamma_mode_active : 1; > > + > > /** > > * @mode: > > * > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > > index 6710b61..b5aa24e 100644 > > --- a/include/drm/drm_file.h > > +++ b/include/drm/drm_file.h > > @@ -201,6 +201,14 @@ struct drm_file { > > bool writeback_connectors; > > > > /** > > + * This is to enable advance gamma modes using > > + * gamma_mode property > > + * > > + * True if client understands advance gamma > > + */ > > + bool advance_gamma_mode_active : 1; > > + > > + /** > > * @is_master: > > * > > * This client is the creator of @master. Protected by > > struct > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 236b01a..abef966 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -696,6 +696,8 @@ struct drm_get_cap { > > */ > > #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5 > > > > +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES 6 > > + > > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > > struct drm_set_client_cap { > > __u64 capability;
mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank: > > -----Original Message----- > > From: Lankhorst, Maarten > > Sent: Monday, April 15, 2019 4:28 PM > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt > > op.org; dri- > > devel@lists.freedesktop.org > > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail. > > com; > > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>; > > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or > > g; > > Sharma, Shashank <shashank.sharma@intel.com> > > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode > > > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > > Introduced a client cap for advance cap mode > > > capability. Userspace should set this to get > > > to be able to use the new gamma_mode property. > > > > > > If this is not set, driver will work in legacy > > > mode. > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > Nack, this doesn't seem like a sensible idea. We already guard it > > behind the gamma mode property. Userspace shouldn't set the gamma > > mode > > to a value it doesn't understand. > > > > ~Maarten > > Hey Maarten, > In that case, what do you suggest should be the right way to do this > ? > > @Ville, any comments here ? > I would say drop this patch, and just enable segmented gamma unconditionally, it's not the first property that can cause trouble when not understood. ~Maarten
On 4/15/2019 7:42 PM, Lankhorst, Maarten wrote: > mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank: >>> -----Original Message----- >>> From: Lankhorst, Maarten >>> Sent: Monday, April 15, 2019 4:28 PM >>> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt >>> op.org; dri- >>> devel@lists.freedesktop.org >>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail. >>> com; >>> sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>; >>> seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or >>> g; >>> Sharma, Shashank <shashank.sharma@intel.com> >>> Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode >>> >>> fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: >>>> Introduced a client cap for advance cap mode >>>> capability. Userspace should set this to get >>>> to be able to use the new gamma_mode property. >>>> >>>> If this is not set, driver will work in legacy >>>> mode. >>>> >>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >>> Nack, this doesn't seem like a sensible idea. We already guard it >>> behind the gamma mode property. Userspace shouldn't set the gamma >>> mode >>> to a value it doesn't understand. >>> >>> ~Maarten >> Hey Maarten, >> In that case, what do you suggest should be the right way to do this >> ? >> >> @Ville, any comments here ? >> > I would say drop this patch, and just enable segmented gamma > unconditionally, it's not the first property that can cause trouble > when not understood. Honestly, I kindof like this simple approach suggestion, which makes the property focused, and easy to use. But one of the benefits I see for this new gamma mode property was, that we can handle other advanced gamma modes like 10/12/split and multi-segmented gamma modes too, using this. So even though the single property is easy to use, but if we try to cover each of the gamma modes per single property, it might get confusing to see 4 different gamma mode properties, and how to set one of this. Another problem I see is, the precision and no of entries in the LUT, for multi-segment gamma is different than traditional gamma, and we might break some (most ?) of the old userspaces. Do you think is there any way in which we can avoid this ? - Shashank > ~Maarten
On Mon, Apr 15, 2019 at 4:14 PM Lankhorst, Maarten <maarten.lankhorst@intel.com> wrote: > > mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank: > > > -----Original Message----- > > > From: Lankhorst, Maarten > > > Sent: Monday, April 15, 2019 4:28 PM > > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt > > > op.org; dri- > > > devel@lists.freedesktop.org > > > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail. > > > com; > > > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>; > > > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or > > > g; > > > Sharma, Shashank <shashank.sharma@intel.com> > > > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode > > > > > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > > > Introduced a client cap for advance cap mode > > > > capability. Userspace should set this to get > > > > to be able to use the new gamma_mode property. > > > > > > > > If this is not set, driver will work in legacy > > > > mode. > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > > > Nack, this doesn't seem like a sensible idea. We already guard it > > > behind the gamma mode property. Userspace shouldn't set the gamma > > > mode > > > to a value it doesn't understand. > > > > > > ~Maarten > > > > Hey Maarten, > > In that case, what do you suggest should be the right way to do this > > ? > > > > @Ville, any comments here ? > > > I would say drop this patch, and just enable segmented gamma > unconditionally, it's not the first property that can cause trouble > when not understood. Yeah, thus far we went with "new properties should have the old behaviour as default, no cap/flag needed". If you mix old&new userspace and stuff starts looking funny, that's not a regression imo. Also, it's a very uncommon use-case. Wrt reset to default: fbdev emulation should do that for anything that's too fancy, which is generally enough for the "developer of compositors" use case. That part might be missing in the gamma/ctm support in general I think. -Daniel
mån 2019-04-15 klockan 15:43 +0300 skrev Ville Syrjälä: > On Mon, Apr 15, 2019 at 10:57:52AM +0000, Lankhorst, Maarten wrote: > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > > Introduced a client cap for advance cap mode > > > capability. Userspace should set this to get > > > to be able to use the new gamma_mode property. > > > > > > If this is not set, driver will work in legacy > > > mode. > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > Nack, this doesn't seem like a sensible idea. We already guard it > > behind the gamma mode property. Userspace shouldn't set the gamma > > mode > > to a value it doesn't understand. > > Old userspace wouldn't know what a gamma_mode prop is. While a client > cap isn't an entirely satisfactory solution I can't think of a better > way at the moment. > > Well, maybe the old "hey kernel, please reset all my props to some > sane default" idea could be another way to deal with this sort of > stuff. Yes, but this approach wouldn't work, lot of other properties that can cause problems when not reset, like plane alpha and blend mode. I don't see why gamma is special in that case. If userspace did set it to some special value, then presumably it knows how to reset it too. It would be different if the split gamma mode was the default. Then I would understand this, but right now this would even break s/r. ~Maarten
On Mon, Apr 15, 2019 at 09:20:55PM +0200, Daniel Vetter wrote: > On Mon, Apr 15, 2019 at 4:14 PM Lankhorst, Maarten > <maarten.lankhorst@intel.com> wrote: > > > > mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank: > > > > -----Original Message----- > > > > From: Lankhorst, Maarten > > > > Sent: Monday, April 15, 2019 4:28 PM > > > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt > > > > op.org; dri- > > > > devel@lists.freedesktop.org > > > > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail. > > > > com; > > > > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>; > > > > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or > > > > g; > > > > Sharma, Shashank <shashank.sharma@intel.com> > > > > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode > > > > > > > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar: > > > > > Introduced a client cap for advance cap mode > > > > > capability. Userspace should set this to get > > > > > to be able to use the new gamma_mode property. > > > > > > > > > > If this is not set, driver will work in legacy > > > > > mode. > > > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > > > > > Nack, this doesn't seem like a sensible idea. We already guard it > > > > behind the gamma mode property. Userspace shouldn't set the gamma > > > > mode > > > > to a value it doesn't understand. > > > > > > > > ~Maarten > > > > > > Hey Maarten, > > > In that case, what do you suggest should be the right way to do this > > > ? > > > > > > @Ville, any comments here ? > > > > > I would say drop this patch, and just enable segmented gamma > > unconditionally, it's not the first property that can cause trouble > > when not understood. > > Yeah, thus far we went with "new properties should have the old > behaviour as default, no cap/flag needed". That's a given. But it doesn't help with the mixed userspace when one of them doesn't know how to reset it back to default. > If you mix old&new > userspace and stuff starts looking funny, that's not a regression imo. > Also, it's a very uncommon use-case. I was mainly thinking of eg. wayland vs. X type of cases where the user might want to switch. If the wayland compositor uses the new fancy gamma thing but the xserver doesn't then this won't work anymore. But I guess easier solution is to just add the relevant knowledge to the ddx. Which should probably be a hard requirement for landing this. > > Wrt reset to default: fbdev emulation should do that for anything > that's too fancy, which is generally enough for the "developer of > compositors" use case. That part might be missing in the gamma/ctm > support in general I think. Probably. IIRC even running X at 8bpp and then swithcing back to fbcon leaves you with a gargbled palette.
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index d85e0c9..590c87a 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, break; } + crtc_state->advance_gamma_mode_active = + state->advance_gamma_mode_active; ret = drm_atomic_crtc_set_property(crtc, crtc_state, prop, prop_value); break; @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); state->acquire_ctx = &ctx; state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); + state->advance_gamma_mode_active = file_priv->advance_gamma_mode_active; retry: copied_objs = 0; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index d337f16..e593a4c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ return -EINVAL; file_priv->writeback_connectors = req->value; break; + case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES: + if (req->value > 1) + return -EINVAL; + file_priv->advance_gamma_mode_active = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 824a5ed..02c1a68 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -338,6 +338,7 @@ struct drm_atomic_state { * states. */ bool duplicated : 1; + bool advance_gamma_mode_active : 1; struct __drm_planes_state *planes; struct __drm_crtcs_state *crtcs; int num_connector; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f789359..f11dc25 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -170,6 +170,11 @@ struct drm_crtc_state { bool color_mgmt_changed : 1; /** + * This is to indicate advance gamma mode support + */ + bool advance_gamma_mode_active : 1; + + /** * @no_vblank: * * Reflects the ability of a CRTC to send VBLANK events. This state @@ -952,6 +957,8 @@ struct drm_crtc { */ bool enabled; + bool advance_gamma_mode_active : 1; + /** * @mode: * diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 6710b61..b5aa24e 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -201,6 +201,14 @@ struct drm_file { bool writeback_connectors; /** + * This is to enable advance gamma modes using + * gamma_mode property + * + * True if client understands advance gamma + */ + bool advance_gamma_mode_active : 1; + + /** * @is_master: * * This client is the creator of @master. Protected by struct diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 236b01a..abef966 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -696,6 +696,8 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5 +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES 6 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability;
Introduced a client cap for advance cap mode capability. Userspace should set this to get to be able to use the new gamma_mode property. If this is not set, driver will work in legacy mode. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 3 +++ drivers/gpu/drm/drm_ioctl.c | 5 +++++ include/drm/drm_atomic.h | 1 + include/drm/drm_crtc.h | 7 +++++++ include/drm/drm_file.h | 8 ++++++++ include/uapi/drm/drm.h | 2 ++ 6 files changed, 26 insertions(+)