diff mbox series

[RFC,v1,1/7] drm/i915: Add gamma mode property

Message ID 1552984218-3357-2-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add Multi Segment Gamma Support | expand

Commit Message

Shankar, Uma March 19, 2019, 8:30 a.m. UTC
Gen platforms support multiple gamma modes, currently
it's hard coded to operate only in 1 specific mode.
This patch adds a property to make gamma mode programmable.
User can select which mode should be used for a particular
usecase or scenario.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  3 +++
 3 files changed, 51 insertions(+)

Comments

Matt Roper March 21, 2019, 9:19 p.m. UTC | #1
On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> Gen platforms support multiple gamma modes, currently
> it's hard coded to operate only in 1 specific mode.
> This patch adds a property to make gamma mode programmable.
> User can select which mode should be used for a particular
> usecase or scenario.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

I haven't reviewed the series in depth, but I'm a bit confused on how
userspace would approach using this property.  This seems to be exposing
hardware implementation details that I wouldn't expect userspace to need
to worry about (plus I don't think any of the property values here
convey any specific meaning to someone who hasn't read the Intel
bspec/PRM).  E.g., why does userspace care about "split gamma" when the
driver takes care of the programming details and userspace never sees
the actual way the registers are laid out and written?

My understanding is that what really matters is how many table entries
there are for userspace to fill in, what input range(s) they cover, and
how the bits of each table entry are interpreted.  I think we'd want to
handle this in a vendor-agnostic way in the DRM core if possible; most
of the display servers that get used these days are cross-platform and
probably won't want to add Intel-specific logic (or platform-specific
logic if we wind up with a different set of options on future Intel
platforms).

> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c65c2e6..02231ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> +	struct drm_property *gamma_mode_property;
> +
>  	/* hda/i915 audio component */
>  	struct i915_audio_component *audio_component;
>  	bool audio_component_registered;
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a..9d43d19 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -92,6 +92,19 @@
>  	0x0800, 0x0100, 0x0800,
>  };
>  
> +#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
> +#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
> +#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
> +#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
> +
> +#define INTEL_GAMMA_MODE_MASK (\
> +		LEGACY_PALETTE_MODE_8BIT | \
> +		PRECISION_PALETTE_MODE_10BIT | \
> +		INTERPOLATED_GAMMA_MODE_12BIT | \
> +		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
> +		BIT_SPLIT_GAMMA_MODE_12BIT)

Is the "BIT_" prefix on this last one a typo?  I assume this was
supposed to just be the SPLIT_GAMMA_MODE_12BIT defined above?

> +
>  static bool lut_is_legacy(const struct drm_property_blob *lut)
>  {
>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
> @@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
>  		lut_is_legacy(crtc_state->base.gamma_lut);
>  }
>  
> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
> +	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
> +	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
> +	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
> +	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
> +			"12 Bit Multi Segmented Gamma Mode" },
> +	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
> +};
> +
> +void
> +intel_attach_gamma_mode_property(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_property *prop;
> +
> +	prop = dev_priv->gamma_mode_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +						"Gamma Mode",
> +						gamma_mode_supported,
> +						ARRAY_SIZE(gamma_mode_supported));

If we do expose hardware-specific gamma modes like this, then I think
we'd want to create this property with a platform-specific list of
modes so that userspace doesn't even have the options for modes that
aren't supported on the platform they're running on.

> +		if (!prop)
> +			return;
> +
> +		dev_priv->gamma_mode_property = prop;
> +	}
> +
> +	drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
>  /*
>   * When using limited range, multiply the matrix given by userspace by
>   * the matrix that we would use for the limited range.
> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>  					   INTEL_INFO(dev_priv)->color.degamma_lut_size,
>  					   true,
>  					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
> +
> +	intel_attach_gamma_mode_property(crtc);

It looks like we're exposing the property to userspace in this patch,
but we don't finish wiring up the functionality until later patches in
the series; that's going to make things confusing if someone bisects
over this range of patches.  It would be best to hold off on exposing
new interfaces like this to userspace until the end of the
implementation when they're fully functional.


Matt

>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d9f188e..fd84fe9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>  	u8 nv12_planes;
>  	u8 c8_planes;
>  
> +	/* Gamma mode type programmed on the pipe */
> +	u32 gamma_mode_type;
> +
>  	/* bitmask of planes that will be updated during the commit */
>  	u8 update_planes;
>  
> -- 
> 1.9.1
>
Shankar, Uma March 22, 2019, 1:06 p.m. UTC | #2
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 2:50 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
>
>On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
>> Gen platforms support multiple gamma modes, currently it's hard coded
>> to operate only in 1 specific mode.
>> This patch adds a property to make gamma mode programmable.
>> User can select which mode should be used for a particular usecase or
>> scenario.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>
>I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
>approach using this property.  This seems to be exposing hardware implementation
>details that I wouldn't expect userspace to need to worry about (plus I don't think any
>of the property values here convey any specific meaning to someone who hasn't read
>the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
>driver takes care of the programming details and userspace never sees the actual way
>the registers are laid out and written?
>My understanding is that what really matters is how many table entries there are for
>userspace to fill in, what input range(s) they cover, and how the bits of each table
>entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
>DRM core if possible; most of the display servers that get used these days are cross-
>platform and probably won't want to add Intel-specific logic (or platform-specific
>logic if we wind up with a different set of options on future Intel platforms).

Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
documentation for the usage of the property. 

@Ville- What do you recommend or suggest for these interfaces.

>> ---
>>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>>  drivers/gpu/drm/i915/intel_color.c | 46
>++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>>  3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index c65c2e6..02231ae 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>>  	struct drm_property *broadcast_rgb_property;
>>  	struct drm_property *force_audio_property;
>>
>> +	struct drm_property *gamma_mode_property;
>> +
>>  	/* hda/i915 audio component */
>>  	struct i915_audio_component *audio_component;
>>  	bool audio_component_registered;
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 467fd1a..9d43d19 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -92,6 +92,19 @@
>>  	0x0800, 0x0100, 0x0800,
>>  };
>>
>> +#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
>> +#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
>> +#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
>> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
>> +#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
>> +
>> +#define INTEL_GAMMA_MODE_MASK (\
>> +		LEGACY_PALETTE_MODE_8BIT | \
>> +		PRECISION_PALETTE_MODE_10BIT | \
>> +		INTERPOLATED_GAMMA_MODE_12BIT | \
>> +		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
>> +		BIT_SPLIT_GAMMA_MODE_12BIT)
>
>Is the "BIT_" prefix on this last one a typo?  I assume this was supposed to just be the
>SPLIT_GAMMA_MODE_12BIT defined above?

Yes, will fix this.

>> +
>>  static bool lut_is_legacy(const struct drm_property_blob *lut)  {
>>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -105,6
>> +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state
>*crtc_state
>>  		lut_is_legacy(crtc_state->base.gamma_lut);
>>  }
>>
>> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
>> +	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
>> +	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
>> +	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma
>Mode" },
>> +	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
>> +			"12 Bit Multi Segmented Gamma Mode" },
>> +	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" }, };
>> +
>> +void
>> +intel_attach_gamma_mode_property(struct intel_crtc *crtc) {
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct drm_property *prop;
>> +
>> +	prop = dev_priv->gamma_mode_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> +						"Gamma Mode",
>> +						gamma_mode_supported,
>> +
>	ARRAY_SIZE(gamma_mode_supported));
>
>If we do expose hardware-specific gamma modes like this, then I think we'd want to
>create this property with a platform-specific list of modes so that userspace doesn't
>even have the options for modes that aren't supported on the platform they're
>running on.

Ok, will add the property enum based on platform  capabilities.

>> +		if (!prop)
>> +			return;
>> +
>> +		dev_priv->gamma_mode_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&crtc->base.base, prop, 0); }
>> +
>>  /*
>>   * When using limited range, multiply the matrix given by userspace by
>>   * the matrix that we would use for the limited range.
>> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>>  					   INTEL_INFO(dev_priv)-
>>color.degamma_lut_size,
>>  					   true,
>>  					   INTEL_INFO(dev_priv)-
>>color.gamma_lut_size);
>> +
>> +	intel_attach_gamma_mode_property(crtc);
>
>It looks like we're exposing the property to userspace in this patch, but we don't finish
>wiring up the functionality until later patches in the series; that's going to make things
>confusing if someone bisects over this range of patches.  It would be best to hold off
>on exposing new interfaces like this to userspace until the end of the implementation
>when they're fully functional.

Ok, will move the attach to a later patch when all the necessary ingredients are in place.

Regards,
Uma Shankar

>
>Matt
>
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d9f188e..fd84fe9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>>  	u8 nv12_planes;
>>  	u8 c8_planes;
>>
>> +	/* Gamma mode type programmed on the pipe */
>> +	u32 gamma_mode_type;
>> +
>>  	/* bitmask of planes that will be updated during the commit */
>>  	u8 update_planes;
>>
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
Brian Starkey March 22, 2019, 1:39 p.m. UTC | #3
Hi,

On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Friday, March 22, 2019 2:50 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> >Shashank <shashank.sharma@intel.com>
> >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> >
> >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> >> Gen platforms support multiple gamma modes, currently it's hard coded
> >> to operate only in 1 specific mode.
> >> This patch adds a property to make gamma mode programmable.
> >> User can select which mode should be used for a particular usecase or
> >> scenario.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >
> >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> >approach using this property.  This seems to be exposing hardware implementation
> >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> >of the property values here convey any specific meaning to someone who hasn't read
> >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> >driver takes care of the programming details and userspace never sees the actual way
> >the registers are laid out and written?
> >My understanding is that what really matters is how many table entries there are for
> >userspace to fill in, what input range(s) they cover, and how the bits of each table
> >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> >DRM core if possible; most of the display servers that get used these days are cross-
> >platform and probably won't want to add Intel-specific logic (or platform-specific
> >logic if we wind up with a different set of options on future Intel platforms).
> 
> Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> documentation for the usage of the property. 
> 
> @Ville- What do you recommend or suggest for these interfaces.

Just to add to the conversation - some of our HW has fixed LUTs, which
isn't really very well exposed by the current UAPI. We do it by having
the kernel driver just look at the userspace-provided blob, and it if
matches the fixed curve we accept it.

I thought one way to expose this would be to have kernel-created blobs
representing the fixed LUTs, which userspace could query to figure out
what LUTs were available.

That might not need to interact with the proposals here, but perhaps
if there's going to be some kind kind of gamma-mode enumeration, fixed
LUTs could be considered there, too.

Cheers,
-Brian
Ville Syrjälä March 22, 2019, 2:02 p.m. UTC | #4
On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> Hi,
> 
> On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Roper, Matthew D
> > >Sent: Friday, March 22, 2019 2:50 AM
> > >To: Shankar, Uma <uma.shankar@intel.com>
> > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > >Shashank <shashank.sharma@intel.com>
> > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > >
> > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > >> to operate only in 1 specific mode.
> > >> This patch adds a property to make gamma mode programmable.
> > >> User can select which mode should be used for a particular usecase or
> > >> scenario.
> > >>
> > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > >
> > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > >approach using this property.  This seems to be exposing hardware implementation
> > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > >of the property values here convey any specific meaning to someone who hasn't read
> > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > >driver takes care of the programming details and userspace never sees the actual way
> > >the registers are laid out and written?
> > >My understanding is that what really matters is how many table entries there are for
> > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > >DRM core if possible; most of the display servers that get used these days are cross-
> > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > >logic if we wind up with a different set of options on future Intel platforms).
> > 
> > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > documentation for the usage of the property. 
> > 
> > @Ville- What do you recommend or suggest for these interfaces.
> 
> Just to add to the conversation - some of our HW has fixed LUTs, which
> isn't really very well exposed by the current UAPI. We do it by having
> the kernel driver just look at the userspace-provided blob, and it if
> matches the fixed curve we accept it.

So you just have say "Use the sRGB curve" bit in some register etc.?

I think we should be able to integrate that somehow into my design:
https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       

Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
just add another member into the struct to indicate which curve it
is. And we could embed the fixed blob ID in there as well. That way
userspace wouldn't even have to actually get the blob for the curve.
Brian Starkey March 22, 2019, 3:42 p.m. UTC | #5
On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > Hi,
> > 
> > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > 
> > > 
> > > >-----Original Message-----
> > > >From: Roper, Matthew D
> > > >Sent: Friday, March 22, 2019 2:50 AM
> > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > >Shashank <shashank.sharma@intel.com>
> > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > >
> > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > >> to operate only in 1 specific mode.
> > > >> This patch adds a property to make gamma mode programmable.
> > > >> User can select which mode should be used for a particular usecase or
> > > >> scenario.
> > > >>
> > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > >
> > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > >approach using this property.  This seems to be exposing hardware implementation
> > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > >of the property values here convey any specific meaning to someone who hasn't read
> > > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > > >driver takes care of the programming details and userspace never sees the actual way
> > > >the registers are laid out and written?
> > > >My understanding is that what really matters is how many table entries there are for
> > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > >logic if we wind up with a different set of options on future Intel platforms).
> > > 
> > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > documentation for the usage of the property. 
> > > 
> > > @Ville- What do you recommend or suggest for these interfaces.
> > 
> > Just to add to the conversation - some of our HW has fixed LUTs, which
> > isn't really very well exposed by the current UAPI. We do it by having
> > the kernel driver just look at the userspace-provided blob, and it if
> > matches the fixed curve we accept it.
> 
> So you just have say "Use the sRGB curve" bit in some register etc.?

Yep, precisely. In the future, maybe multiple fixed LUTs to choose
from.

> 
> I think we should be able to integrate that somehow into my design:
> https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       
> 
> Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> just add another member into the struct to indicate which curve it
> is. And we could embed the fixed blob ID in there as well. That way
> userspace wouldn't even have to actually get the blob for the curve.

Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
interface, so it certainly could fit in there.

If I understand the code correctly, each value in the enum list is the
ID of a blob, and userspace can query that blob to figure out what the
entry means (instead of needing _really really long_ descriptive
strings).

I wonder if that property type in general is a little _too_ rich. I
worry that it would be easy to just defer loads of vendor-specific
detail into the blob, making the API look "generic" on the surface,
when really it's effectively a list of vendor-defined (void *)s.

...that said, in some cases vendor-defined (void *)s is what we might
want (e.g. scaler coefficient tables).

Still looks like a neat idea, perhaps just needs some policy.

Thanks,
-Brian

> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä March 22, 2019, 3:51 p.m. UTC | #6
On Fri, Mar 22, 2019 at 03:42:43PM +0000, Brian Starkey wrote:
> On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > > Hi,
> > > 
> > > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > > 
> > > > 
> > > > >-----Original Message-----
> > > > >From: Roper, Matthew D
> > > > >Sent: Friday, March 22, 2019 2:50 AM
> > > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > > >Shashank <shashank.sharma@intel.com>
> > > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > > >
> > > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > > >> to operate only in 1 specific mode.
> > > > >> This patch adds a property to make gamma mode programmable.
> > > > >> User can select which mode should be used for a particular usecase or
> > > > >> scenario.
> > > > >>
> > > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > > >
> > > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > > >approach using this property.  This seems to be exposing hardware implementation
> > > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > > >of the property values here convey any specific meaning to someone who hasn't read
> > > > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > > > >driver takes care of the programming details and userspace never sees the actual way
> > > > >the registers are laid out and written?
> > > > >My understanding is that what really matters is how many table entries there are for
> > > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > > >logic if we wind up with a different set of options on future Intel platforms).
> > > > 
> > > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > > documentation for the usage of the property. 
> > > > 
> > > > @Ville- What do you recommend or suggest for these interfaces.
> > > 
> > > Just to add to the conversation - some of our HW has fixed LUTs, which
> > > isn't really very well exposed by the current UAPI. We do it by having
> > > the kernel driver just look at the userspace-provided blob, and it if
> > > matches the fixed curve we accept it.
> > 
> > So you just have say "Use the sRGB curve" bit in some register etc.?
> 
> Yep, precisely. In the future, maybe multiple fixed LUTs to choose
> from.
> 
> > 
> > I think we should be able to integrate that somehow into my design:
> > https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       
> > 
> > Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> > just add another member into the struct to indicate which curve it
> > is. And we could embed the fixed blob ID in there as well. That way
> > userspace wouldn't even have to actually get the blob for the curve.
> 
> Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
> interface, so it certainly could fit in there.
> 
> If I understand the code correctly, each value in the enum list is the
> ID of a blob, and userspace can query that blob to figure out what the
> entry means (instead of needing _really really long_ descriptive
> strings).
> 
> I wonder if that property type in general is a little _too_ rich. I
> worry that it would be easy to just defer loads of vendor-specific
> detail into the blob, making the API look "generic" on the surface,
> when really it's effectively a list of vendor-defined (void *)s.
> 
> ...that said, in some cases vendor-defined (void *)s is what we might
> want (e.g. scaler coefficient tables).
> 
> Still looks like a neat idea, perhaps just needs some policy.

Yeah, I couldn't come up with anything better really. The options that
I see are as you say really long descriptive string, or we'd have to
update all userspace whenever a new enum value is added so that it
can decide what to do with it.

If we go with this idea, then I would definitely want to NAK any patch
that tries to abuse this for "we just need to expose these random
piles of hardware specific data".
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c65c2e6..02231ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1735,6 +1735,8 @@  struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
+	struct drm_property *gamma_mode_property;
+
 	/* hda/i915 audio component */
 	struct i915_audio_component *audio_component;
 	bool audio_component_registered;
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 467fd1a..9d43d19 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -92,6 +92,19 @@ 
 	0x0800, 0x0100, 0x0800,
 };
 
+#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
+#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
+#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
+#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
+#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
+
+#define INTEL_GAMMA_MODE_MASK (\
+		LEGACY_PALETTE_MODE_8BIT | \
+		PRECISION_PALETTE_MODE_10BIT | \
+		INTERPOLATED_GAMMA_MODE_12BIT | \
+		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
+		BIT_SPLIT_GAMMA_MODE_12BIT)
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -105,6 +118,37 @@  static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
 		lut_is_legacy(crtc_state->base.gamma_lut);
 }
 
+static const struct drm_prop_enum_list gamma_mode_supported[] = {
+	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
+	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
+	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
+	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
+			"12 Bit Multi Segmented Gamma Mode" },
+	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
+};
+
+void
+intel_attach_gamma_mode_property(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_property *prop;
+
+	prop = dev_priv->gamma_mode_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+						"Gamma Mode",
+						gamma_mode_supported,
+						ARRAY_SIZE(gamma_mode_supported));
+		if (!prop)
+			return;
+
+		dev_priv->gamma_mode_property = prop;
+	}
+
+	drm_object_attach_property(&crtc->base.base, prop, 0);
+}
+
 /*
  * When using limited range, multiply the matrix given by userspace by
  * the matrix that we would use for the limited range.
@@ -907,4 +951,6 @@  void intel_color_init(struct intel_crtc *crtc)
 					   INTEL_INFO(dev_priv)->color.degamma_lut_size,
 					   true,
 					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
+
+	intel_attach_gamma_mode_property(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d9f188e..fd84fe9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1034,6 +1034,9 @@  struct intel_crtc_state {
 	u8 nv12_planes;
 	u8 c8_planes;
 
+	/* Gamma mode type programmed on the pipe */
+	u32 gamma_mode_type;
+
 	/* bitmask of planes that will be updated during the commit */
 	u8 update_planes;