[v3,6/7] drm: Add Client Cap for advance gamma mode
diff mbox series

Message ID 1555064463-18479-7-git-send-email-uma.shankar@intel.com
State New
Headers show
Series
  • Add Multi Segment Gamma Support
Related show

Commit Message

Shankar, Uma April 12, 2019, 10:21 a.m. UTC
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(+)

Comments

Lankhorst, Maarten April 15, 2019, 10:57 a.m. UTC | #1
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;
Ville Syrjälä April 15, 2019, 12:43 p.m. UTC | #2
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
Sharma, Shashank April 15, 2019, 1:56 p.m. UTC | #3
> -----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;
Lankhorst, Maarten April 15, 2019, 2:12 p.m. UTC | #4
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
Sharma, Shashank April 15, 2019, 2:29 p.m. UTC | #5
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
Daniel Vetter April 15, 2019, 7:20 p.m. UTC | #6
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
Lankhorst, Maarten April 16, 2019, 8:54 a.m. UTC | #7
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
Ville Syrjälä April 16, 2019, 3:06 p.m. UTC | #8
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.

Patch
diff mbox series

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;