| 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 |
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 >
>-----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
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
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.
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
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 --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;
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(+)