| Message ID | 1552984218-3357-4-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Multi Segment Gamma Support | expand |
tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: > Multi Segment Gamma Mode is added in Gen11+ platforms. > Added a property interface to enable that. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 02231ae..f20d418 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1736,6 +1736,7 @@ struct drm_i915_private { > struct drm_property *force_audio_property; > > struct drm_property *gamma_mode_property; > + struct drm_property *multi_segment_gamma_mode_property; Seems to me both properties should be part of drm core? > /* hda/i915 audio component */ > struct i915_audio_component *audio_component; > diff --git a/drivers/gpu/drm/i915/intel_color.c > b/drivers/gpu/drm/i915/intel_color.c > index 9d43d19..399d63d 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const > struct intel_crtc_state *crtc_state > drm_object_attach_property(&crtc->base.base, prop, 0); > } > > +void > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; > + if (!prop) { > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > + "Multi-segment Gamma", > 0); > + if (!prop) > + return; > + > + dev_priv->multi_segment_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. > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) > INTEL_INFO(dev_priv)- > >color.gamma_lut_size); > > intel_attach_gamma_mode_property(crtc); > + > + if (INTEL_GEN(dev_priv) >= 11) > + intel_attach_multi_segment_gamma_mode_property(crtc) > ; > } > diff --git a/include/uapi/drm/i915_drm.h > b/include/uapi/drm/i915_drm.h > index aa2d4c7..8f1974e 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { > __u8 data[]; > }; > > +/* > + * Structure for muti segmented gamma lut > + */ > +struct multi_segment_gamma_lut { > + /* Number of Lut Segments */ > + __u8 segment_cnt; > + /* Precison of LUT entries in bits */ > + __u8 precision_bits; > + /* Pointer having number of LUT elements in each segment */ > + __u32 *segment_lut_cnt_ptr; > + /* Pointer to store exact lut values for each segment */ > + __u32 *segment_lut_ptr; > +}; > And perhaps a variation of this as description for all gamma mode types.
On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: > tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: > > Multi Segment Gamma Mode is added in Gen11+ platforms. > > Added a property interface to enable that. > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ > > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 02231ae..f20d418 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1736,6 +1736,7 @@ struct drm_i915_private { > > struct drm_property *force_audio_property; > > > > struct drm_property *gamma_mode_property; > > + struct drm_property *multi_segment_gamma_mode_property; > > Seems to me both properties should be part of drm core? > > > /* hda/i915 audio component */ > > struct i915_audio_component *audio_component; > > diff --git a/drivers/gpu/drm/i915/intel_color.c > > b/drivers/gpu/drm/i915/intel_color.c > > index 9d43d19..399d63d 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const > > struct intel_crtc_state *crtc_state > > drm_object_attach_property(&crtc->base.base, prop, 0); > > } > > > > +void > > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; > > + if (!prop) { > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > > + "Multi-segment Gamma", > > 0); > > + if (!prop) > > + return; > > + > > + dev_priv->multi_segment_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. > > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) > > INTEL_INFO(dev_priv)- > > >color.gamma_lut_size); > > > > intel_attach_gamma_mode_property(crtc); > > + > > + if (INTEL_GEN(dev_priv) >= 11) > > + intel_attach_multi_segment_gamma_mode_property(crtc) > > ; > > } > > diff --git a/include/uapi/drm/i915_drm.h > > b/include/uapi/drm/i915_drm.h > > index aa2d4c7..8f1974e 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { > > __u8 data[]; > > }; > > > > +/* > > + * Structure for muti segmented gamma lut > > + */ > > +struct multi_segment_gamma_lut { > > + /* Number of Lut Segments */ > > + __u8 segment_cnt; > > + /* Precison of LUT entries in bits */ > > + __u8 precision_bits; > > + /* Pointer having number of LUT elements in each segment */ > > + __u32 *segment_lut_cnt_ptr; > > + /* Pointer to store exact lut values for each segment */ > > + __u32 *segment_lut_ptr; > > +}; > > > And perhaps a variation of this as description for all gamma mode > types. This is my old idea how to represent fancier LUTs: https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95 https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5aa58f Each distinct segment of the curve would be just described by a fixed size range descriptor, and the entire blob would be made up of however many of those are needed. That way we don't even need any multi-segment uapi for setting up the LUT. The blob for that would just contain as many entries as the LUT needs in that specific mode.
>-----Original Message----- >From: Syrjala, Ville >Sent: Tuesday, March 19, 2019 10:29 PM >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D ><matthew.d.roper@intel.com> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode > >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: >> > Multi Segment Gamma Mode is added in Gen11+ platforms. >> > Added a property interface to enable that. >> > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_drv.h | 1 + >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ >> > 3 files changed, 38 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { >> > struct drm_property *force_audio_property; >> > >> > struct drm_property *gamma_mode_property; >> > + struct drm_property *multi_segment_gamma_mode_property; >> >> Seems to me both properties should be part of drm core? Sure Maarten, we can move gamma_mode property to drm. >> >> > /* hda/i915 audio component */ >> > struct i915_audio_component *audio_component; diff --git >> > a/drivers/gpu/drm/i915/intel_color.c >> > b/drivers/gpu/drm/i915/intel_color.c >> > index 9d43d19..399d63d 100644 >> > --- a/drivers/gpu/drm/i915/intel_color.c >> > +++ b/drivers/gpu/drm/i915/intel_color.c >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const >> > struct intel_crtc_state *crtc_state >> > drm_object_attach_property(&crtc->base.base, prop, 0); } >> > >> > +void >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; >> > + if (!prop) { >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, >> > + "Multi-segment Gamma", >> > 0); >> > + if (!prop) >> > + return; >> > + >> > + dev_priv->multi_segment_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. >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) >> > INTEL_INFO(dev_priv)- >> > >color.gamma_lut_size); >> > >> > intel_attach_gamma_mode_property(crtc); >> > + >> > + if (INTEL_GEN(dev_priv) >= 11) >> > + intel_attach_multi_segment_gamma_mode_property(crtc) >> > ; >> > } >> > diff --git a/include/uapi/drm/i915_drm.h >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 >> > --- a/include/uapi/drm/i915_drm.h >> > +++ b/include/uapi/drm/i915_drm.h >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { >> > __u8 data[]; >> > }; >> > >> > +/* >> > + * Structure for muti segmented gamma lut */ struct >> > +multi_segment_gamma_lut { >> > + /* Number of Lut Segments */ >> > + __u8 segment_cnt; >> > + /* Precison of LUT entries in bits */ >> > + __u8 precision_bits; >> > + /* Pointer having number of LUT elements in each segment */ >> > + __u32 *segment_lut_cnt_ptr; >> > + /* Pointer to store exact lut values for each segment */ >> > + __u32 *segment_lut_ptr; >> > +}; >> > >> And perhaps a variation of this as description for all gamma mode >> types. > >This is my old idea how to represent fancier LUTs: >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130 >0ff95 >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a >a58f > >Each distinct segment of the curve would be just described by a fixed size range >descriptor, and the entire blob would be made up of however many of those are >needed. > >That way we don't even need any multi-segment uapi for setting up the LUT. The blob >for that would just contain as many entries as the LUT needs in that specific mode. Hi Ville, This also sounds good. What is your suggestion on this. Any recommendation on how to take this forward. Thanks & Regards, Uma Shankar >-- >Ville Syrjälä >Intel
On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Syrjala, Ville > >Sent: Tuesday, March 19, 2019 10:29 PM > >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> > >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; > >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D > ><matthew.d.roper@intel.com> > >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode > > > >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: > >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: > >> > Multi Segment Gamma Mode is added in Gen11+ platforms. > >> > Added a property interface to enable that. > >> > > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_drv.h | 1 + > >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ > >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > >> > 3 files changed, 38 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { > >> > struct drm_property *force_audio_property; > >> > > >> > struct drm_property *gamma_mode_property; > >> > + struct drm_property *multi_segment_gamma_mode_property; > >> > >> Seems to me both properties should be part of drm core? > > Sure Maarten, we can move gamma_mode property to drm. > > >> > >> > /* hda/i915 audio component */ > >> > struct i915_audio_component *audio_component; diff --git > >> > a/drivers/gpu/drm/i915/intel_color.c > >> > b/drivers/gpu/drm/i915/intel_color.c > >> > index 9d43d19..399d63d 100644 > >> > --- a/drivers/gpu/drm/i915/intel_color.c > >> > +++ b/drivers/gpu/drm/i915/intel_color.c > >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const > >> > struct intel_crtc_state *crtc_state > >> > drm_object_attach_property(&crtc->base.base, prop, 0); } > >> > > >> > +void > >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; > >> > + if (!prop) { > >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > >> > + "Multi-segment Gamma", > >> > 0); > >> > + if (!prop) > >> > + return; > >> > + > >> > + dev_priv->multi_segment_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. > >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) > >> > INTEL_INFO(dev_priv)- > >> > >color.gamma_lut_size); > >> > > >> > intel_attach_gamma_mode_property(crtc); > >> > + > >> > + if (INTEL_GEN(dev_priv) >= 11) > >> > + intel_attach_multi_segment_gamma_mode_property(crtc) > >> > ; > >> > } > >> > diff --git a/include/uapi/drm/i915_drm.h > >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 > >> > --- a/include/uapi/drm/i915_drm.h > >> > +++ b/include/uapi/drm/i915_drm.h > >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { > >> > __u8 data[]; > >> > }; > >> > > >> > +/* > >> > + * Structure for muti segmented gamma lut */ struct > >> > +multi_segment_gamma_lut { > >> > + /* Number of Lut Segments */ > >> > + __u8 segment_cnt; > >> > + /* Precison of LUT entries in bits */ > >> > + __u8 precision_bits; > >> > + /* Pointer having number of LUT elements in each segment */ > >> > + __u32 *segment_lut_cnt_ptr; > >> > + /* Pointer to store exact lut values for each segment */ > >> > + __u32 *segment_lut_ptr; I'm confused how this blob is supposed to be used. It seems like the pointers in here won't be meaningful across the userspace/kernel copy? It also looks like 'segment_lut_ptr' doesn't appear again in this patch series. Like Ville describes below, I'd expect a property describing segmented gamma to basically be a collection of structures that describe a given range of the regular main LUT: the start/end indices of the LUT that hold the segment, how the bits of table entries in this segment should be interpreted, etc. Whatever we come up with, it will definitely be important to add some kerneldoc that describes how the property is used/interpreted. Matt > >> > +}; > >> > > >> And perhaps a variation of this as description for all gamma mode > >> types. > > > >This is my old idea how to represent fancier LUTs: > >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130 > >0ff95 > >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a > >a58f > > > >Each distinct segment of the curve would be just described by a fixed size range > >descriptor, and the entire blob would be made up of however many of those are > >needed. > > > >That way we don't even need any multi-segment uapi for setting up the LUT. The blob > >for that would just contain as many entries as the LUT needs in that specific mode. > > Hi Ville, > This also sounds good. What is your suggestion on this. Any recommendation on how to take this > forward. > > Thanks & Regards, > Uma Shankar > > >-- > >Ville Syrjälä > >Intel
>-----Original Message----- >From: Roper, Matthew D >Sent: Friday, March 22, 2019 3:23 AM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org; Sharma, Shashank ><shashank.sharma@intel.com> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode > >On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote: >> >> >> >-----Original Message----- >> >From: Syrjala, Ville >> >Sent: Tuesday, March 19, 2019 10:29 PM >> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> >> >Cc: Shankar, Uma <uma.shankar@intel.com>; >> >intel-gfx@lists.freedesktop.org; Sharma, Shashank >> ><shashank.sharma@intel.com>; Roper, Matthew D >> ><matthew.d.roper@intel.com> >> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment >> >Gamma Mode >> > >> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: >> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: >> >> > Multi Segment Gamma Mode is added in Gen11+ platforms. >> >> > Added a property interface to enable that. >> >> > >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/i915_drv.h | 1 + >> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ >> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ >> >> > 3 files changed, 38 insertions(+) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 >> >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { >> >> > struct drm_property *force_audio_property; >> >> > >> >> > struct drm_property *gamma_mode_property; >> >> > + struct drm_property *multi_segment_gamma_mode_property; >> >> >> >> Seems to me both properties should be part of drm core? >> >> Sure Maarten, we can move gamma_mode property to drm. >> >> >> >> >> > /* hda/i915 audio component */ >> >> > struct i915_audio_component *audio_component; diff --git >> >> > a/drivers/gpu/drm/i915/intel_color.c >> >> > b/drivers/gpu/drm/i915/intel_color.c >> >> > index 9d43d19..399d63d 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_color.c >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c >> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const >> >> > struct intel_crtc_state *crtc_state >> >> > drm_object_attach_property(&crtc->base.base, prop, 0); } >> >> > >> >> > +void >> >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; >> >> > + if (!prop) { >> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, >> >> > + "Multi-segment Gamma", >> >> > 0); >> >> > + if (!prop) >> >> > + return; >> >> > + >> >> > + dev_priv->multi_segment_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. >> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) >> >> > INTEL_INFO(dev_priv)- >> >> > >color.gamma_lut_size); >> >> > >> >> > intel_attach_gamma_mode_property(crtc); >> >> > + >> >> > + if (INTEL_GEN(dev_priv) >= 11) >> >> > + intel_attach_multi_segment_gamma_mode_property(crtc) >> >> > ; >> >> > } >> >> > diff --git a/include/uapi/drm/i915_drm.h >> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 >> >> > --- a/include/uapi/drm/i915_drm.h >> >> > +++ b/include/uapi/drm/i915_drm.h >> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { >> >> > __u8 data[]; >> >> > }; >> >> > >> >> > +/* >> >> > + * Structure for muti segmented gamma lut */ struct >> >> > +multi_segment_gamma_lut { >> >> > + /* Number of Lut Segments */ >> >> > + __u8 segment_cnt; >> >> > + /* Precison of LUT entries in bits */ >> >> > + __u8 precision_bits; >> >> > + /* Pointer having number of LUT elements in each segment */ >> >> > + __u32 *segment_lut_cnt_ptr; >> >> > + /* Pointer to store exact lut values for each segment */ >> >> > + __u32 *segment_lut_ptr; > >I'm confused how this blob is supposed to be used. It seems like the pointers in here >won't be meaningful across the userspace/kernel copy? >It also looks like 'segment_lut_ptr' doesn't appear again in this patch series. > >Like Ville describes below, I'd expect a property describing segmented gamma to >basically be a collection of structures that describe a given range of the regular main >LUT: the start/end indices of the LUT that hold the segment, how the bits of table >entries in this segment should be interpreted, etc. Ok, will try to align to that approach and come up with the next version for this series. >Whatever we come up with, it will definitely be important to add some kerneldoc that >describes how the property is used/interpreted. Sure, I will add detailed docs explaining the usage and expectation from userspace. Thanks Matt for your comments. Regards, Uma Shankar > > >Matt > >> >> > +}; >> >> > >> >> And perhaps a variation of this as description for all gamma mode >> >> types. >> > >> >This is my old idea how to represent fancier LUTs: >> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179 >> >04c2130 >> >0ff95 >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68 >> >7bb0ae5a >> >a58f >> > >> >Each distinct segment of the curve would be just described by a fixed >> >size range descriptor, and the entire blob would be made up of >> >however many of those are needed. >> > >> >That way we don't even need any multi-segment uapi for setting up the >> >LUT. The blob for that would just contain as many entries as the LUT needs in that >specific mode. >> >> Hi Ville, >> This also sounds good. What is your suggestion on this. Any >> recommendation on how to take this forward. >> >> Thanks & Regards, >> Uma Shankar >> >> >-- >> >Ville Syrjälä >> >Intel > >-- >Matt Roper >Graphics Software Engineer >IoTG Platform Enabling & Development >Intel Corporation >(916) 356-2795
On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Syrjala, Ville > >Sent: Tuesday, March 19, 2019 10:29 PM > >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> > >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; > >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D > ><matthew.d.roper@intel.com> > >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode > > > >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: > >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: > >> > Multi Segment Gamma Mode is added in Gen11+ platforms. > >> > Added a property interface to enable that. > >> > > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_drv.h | 1 + > >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ > >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > >> > 3 files changed, 38 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { > >> > struct drm_property *force_audio_property; > >> > > >> > struct drm_property *gamma_mode_property; > >> > + struct drm_property *multi_segment_gamma_mode_property; > >> > >> Seems to me both properties should be part of drm core? > > Sure Maarten, we can move gamma_mode property to drm. > > >> > >> > /* hda/i915 audio component */ > >> > struct i915_audio_component *audio_component; diff --git > >> > a/drivers/gpu/drm/i915/intel_color.c > >> > b/drivers/gpu/drm/i915/intel_color.c > >> > index 9d43d19..399d63d 100644 > >> > --- a/drivers/gpu/drm/i915/intel_color.c > >> > +++ b/drivers/gpu/drm/i915/intel_color.c > >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const > >> > struct intel_crtc_state *crtc_state > >> > drm_object_attach_property(&crtc->base.base, prop, 0); } > >> > > >> > +void > >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; > >> > + if (!prop) { > >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > >> > + "Multi-segment Gamma", > >> > 0); > >> > + if (!prop) > >> > + return; > >> > + > >> > + dev_priv->multi_segment_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. > >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) > >> > INTEL_INFO(dev_priv)- > >> > >color.gamma_lut_size); > >> > > >> > intel_attach_gamma_mode_property(crtc); > >> > + > >> > + if (INTEL_GEN(dev_priv) >= 11) > >> > + intel_attach_multi_segment_gamma_mode_property(crtc) > >> > ; > >> > } > >> > diff --git a/include/uapi/drm/i915_drm.h > >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 > >> > --- a/include/uapi/drm/i915_drm.h > >> > +++ b/include/uapi/drm/i915_drm.h > >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { > >> > __u8 data[]; > >> > }; > >> > > >> > +/* > >> > + * Structure for muti segmented gamma lut */ struct > >> > +multi_segment_gamma_lut { > >> > + /* Number of Lut Segments */ > >> > + __u8 segment_cnt; > >> > + /* Precison of LUT entries in bits */ > >> > + __u8 precision_bits; > >> > + /* Pointer having number of LUT elements in each segment */ > >> > + __u32 *segment_lut_cnt_ptr; > >> > + /* Pointer to store exact lut values for each segment */ > >> > + __u32 *segment_lut_ptr; > >> > +}; > >> > > >> And perhaps a variation of this as description for all gamma mode > >> types. > > > >This is my old idea how to represent fancier LUTs: > >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130 > >0ff95 > >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a > >a58f > > > >Each distinct segment of the curve would be just described by a fixed size range > >descriptor, and the entire blob would be made up of however many of those are > >needed. > > > >That way we don't even need any multi-segment uapi for setting up the LUT. The blob > >for that would just contain as many entries as the LUT needs in that specific mode. > > Hi Ville, > This also sounds good. What is your suggestion on this. Any recommendation on how to take this > forward. I think my design should be more or less feasible to use for userspace. I didn't actually get as far as to write userspace code for it, but my idea for x11 was something along these lines: 1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc 2) if none was found pick an interpolated gamma mode with input/output bpc >= screen bpc 3) fall back to whatever is left That approach should work for i915 + intel ddx at least. In theory it should be find for generic userspace too, but maybe it would need a few extra tweaks. Also we need fp16 to actually be able to test the interpolated modes fully. We now have that for icl, but I still need to post my "fp16 for everything gen4+" followup series. And after that we need to glue it all together in igt. Anyways, I've not worked on this branch myself in a while because I want to get the current gamma support fixed/cleaned up first. I have at least one more series after the current one gets reviewed. And after that we still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out. So yeah, I'd prefer to make the current stuff sensible before spending time adding fancier stuff on top. But in the meantime if you want to pick up where I left off with this gamma mode I'd be fine with that.
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, March 22, 2019 7:23 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma >Mode > >On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote: >> >> >> >-----Original Message----- >> >From: Syrjala, Ville >> >Sent: Tuesday, March 19, 2019 10:29 PM >> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> >> >Cc: Shankar, Uma <uma.shankar@intel.com>; >> >intel-gfx@lists.freedesktop.org; Sharma, Shashank >> ><shashank.sharma@intel.com>; Roper, Matthew D >> ><matthew.d.roper@intel.com> >> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment >> >Gamma Mode >> > >> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: >> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: >> >> > Multi Segment Gamma Mode is added in Gen11+ platforms. >> >> > Added a property interface to enable that. >> >> > >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/i915_drv.h | 1 + >> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ >> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ >> >> > 3 files changed, 38 insertions(+) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 >> >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { >> >> > struct drm_property *force_audio_property; >> >> > >> >> > struct drm_property *gamma_mode_property; >> >> > + struct drm_property *multi_segment_gamma_mode_property; >> >> >> >> Seems to me both properties should be part of drm core? >> >> Sure Maarten, we can move gamma_mode property to drm. >> >> >> >> >> > /* hda/i915 audio component */ >> >> > struct i915_audio_component *audio_component; diff --git >> >> > a/drivers/gpu/drm/i915/intel_color.c >> >> > b/drivers/gpu/drm/i915/intel_color.c >> >> > index 9d43d19..399d63d 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_color.c >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c >> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const >> >> > struct intel_crtc_state *crtc_state >> >> > drm_object_attach_property(&crtc->base.base, prop, 0); } >> >> > >> >> > +void >> >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; >> >> > + if (!prop) { >> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, >> >> > + "Multi-segment Gamma", >> >> > 0); >> >> > + if (!prop) >> >> > + return; >> >> > + >> >> > + dev_priv->multi_segment_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. >> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) >> >> > INTEL_INFO(dev_priv)- >> >> > >color.gamma_lut_size); >> >> > >> >> > intel_attach_gamma_mode_property(crtc); >> >> > + >> >> > + if (INTEL_GEN(dev_priv) >= 11) >> >> > + intel_attach_multi_segment_gamma_mode_property(crtc) >> >> > ; >> >> > } >> >> > diff --git a/include/uapi/drm/i915_drm.h >> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 >> >> > --- a/include/uapi/drm/i915_drm.h >> >> > +++ b/include/uapi/drm/i915_drm.h >> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { >> >> > __u8 data[]; >> >> > }; >> >> > >> >> > +/* >> >> > + * Structure for muti segmented gamma lut */ struct >> >> > +multi_segment_gamma_lut { >> >> > + /* Number of Lut Segments */ >> >> > + __u8 segment_cnt; >> >> > + /* Precison of LUT entries in bits */ >> >> > + __u8 precision_bits; >> >> > + /* Pointer having number of LUT elements in each segment */ >> >> > + __u32 *segment_lut_cnt_ptr; >> >> > + /* Pointer to store exact lut values for each segment */ >> >> > + __u32 *segment_lut_ptr; >> >> > +}; >> >> > >> >> And perhaps a variation of this as description for all gamma mode >> >> types. >> > >> >This is my old idea how to represent fancier LUTs: >> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179 >> >04c2130 >> >0ff95 >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68 >> >7bb0ae5a >> >a58f >> > >> >Each distinct segment of the curve would be just described by a fixed >> >size range descriptor, and the entire blob would be made up of >> >however many of those are needed. >> > >> >That way we don't even need any multi-segment uapi for setting up the >> >LUT. The blob for that would just contain as many entries as the LUT needs in that >specific mode. >> >> Hi Ville, >> This also sounds good. What is your suggestion on this. Any >> recommendation on how to take this forward. > >I think my design should be more or less feasible to use for userspace. I didn't actually >get as far as to write userspace code for it, but my idea for x11 was something along >these lines: > >1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc >2) if none was found pick an interpolated gamma mode with > input/output bpc >= screen bpc >3) fall back to whatever is left > >That approach should work for i915 + intel ddx at least. In theory it should be find for >generic userspace too, but maybe it would need a few extra tweaks. > >Also we need fp16 to actually be able to test the interpolated modes fully. We now >have that for icl, but I still need to post my "fp16 for everything gen4+" followup >series. And after that we need to glue it all together in igt. > >Anyways, I've not worked on this branch myself in a while because I want to get the >current gamma support fixed/cleaned up first. >I have at least one more series after the current one gets reviewed. And after that we >still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out. > >So yeah, I'd prefer to make the current stuff sensible before spending time adding >fancier stuff on top. But in the meantime if you want to pick up where I left off with >this gamma mode I'd be fine with that. Hi Ville, Thanks for sharing these details and also the design what you have is really nice. I was trying to build on top of it and trying to check the interface before floating the next version. There seems to be a limitation in creating a property with both BLOB and ENUM flags. /* Only one legacy type at a time please */ if (legacy_type && !is_power_of_2(legacy_type)) return false; Any suggestion on how to deal with this limitation. Thanks & Regards, Uma Shankar >-- >Ville Syrjälä >Intel
On Thu, Mar 28, 2019 at 07:00:30PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Friday, March 22, 2019 7:23 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org > >Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma > >Mode > > > >On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote: > >> > >> > >> >-----Original Message----- > >> >From: Syrjala, Ville > >> >Sent: Tuesday, March 19, 2019 10:29 PM > >> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com> > >> >Cc: Shankar, Uma <uma.shankar@intel.com>; > >> >intel-gfx@lists.freedesktop.org; Sharma, Shashank > >> ><shashank.sharma@intel.com>; Roper, Matthew D > >> ><matthew.d.roper@intel.com> > >> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment > >> >Gamma Mode > >> > > >> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote: > >> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar: > >> >> > Multi Segment Gamma Mode is added in Gen11+ platforms. > >> >> > Added a property interface to enable that. > >> >> > > >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >> >> > --- > >> >> > drivers/gpu/drm/i915/i915_drv.h | 1 + > >> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ > >> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > >> >> > 3 files changed, 38 insertions(+) > >> >> > > >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 > >> >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private { > >> >> > struct drm_property *force_audio_property; > >> >> > > >> >> > struct drm_property *gamma_mode_property; > >> >> > + struct drm_property *multi_segment_gamma_mode_property; > >> >> > >> >> Seems to me both properties should be part of drm core? > >> > >> Sure Maarten, we can move gamma_mode property to drm. > >> > >> >> > >> >> > /* hda/i915 audio component */ > >> >> > struct i915_audio_component *audio_component; diff --git > >> >> > a/drivers/gpu/drm/i915/intel_color.c > >> >> > b/drivers/gpu/drm/i915/intel_color.c > >> >> > index 9d43d19..399d63d 100644 > >> >> > --- a/drivers/gpu/drm/i915/intel_color.c > >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c > >> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const > >> >> > struct intel_crtc_state *crtc_state > >> >> > drm_object_attach_property(&crtc->base.base, prop, 0); } > >> >> > > >> >> > +void > >> >> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; > >> >> > + if (!prop) { > >> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > >> >> > + "Multi-segment Gamma", > >> >> > 0); > >> >> > + if (!prop) > >> >> > + return; > >> >> > + > >> >> > + dev_priv->multi_segment_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. > >> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) > >> >> > INTEL_INFO(dev_priv)- > >> >> > >color.gamma_lut_size); > >> >> > > >> >> > intel_attach_gamma_mode_property(crtc); > >> >> > + > >> >> > + if (INTEL_GEN(dev_priv) >= 11) > >> >> > + intel_attach_multi_segment_gamma_mode_property(crtc) > >> >> > ; > >> >> > } > >> >> > diff --git a/include/uapi/drm/i915_drm.h > >> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 > >> >> > --- a/include/uapi/drm/i915_drm.h > >> >> > +++ b/include/uapi/drm/i915_drm.h > >> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { > >> >> > __u8 data[]; > >> >> > }; > >> >> > > >> >> > +/* > >> >> > + * Structure for muti segmented gamma lut */ struct > >> >> > +multi_segment_gamma_lut { > >> >> > + /* Number of Lut Segments */ > >> >> > + __u8 segment_cnt; > >> >> > + /* Precison of LUT entries in bits */ > >> >> > + __u8 precision_bits; > >> >> > + /* Pointer having number of LUT elements in each segment */ > >> >> > + __u32 *segment_lut_cnt_ptr; > >> >> > + /* Pointer to store exact lut values for each segment */ > >> >> > + __u32 *segment_lut_ptr; > >> >> > +}; > >> >> > > >> >> And perhaps a variation of this as description for all gamma mode > >> >> types. > >> > > >> >This is my old idea how to represent fancier LUTs: > >> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179 > >> >04c2130 > >> >0ff95 > >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68 > >> >7bb0ae5a > >> >a58f > >> > > >> >Each distinct segment of the curve would be just described by a fixed > >> >size range descriptor, and the entire blob would be made up of > >> >however many of those are needed. > >> > > >> >That way we don't even need any multi-segment uapi for setting up the > >> >LUT. The blob for that would just contain as many entries as the LUT needs in that > >specific mode. > >> > >> Hi Ville, > >> This also sounds good. What is your suggestion on this. Any > >> recommendation on how to take this forward. > > > >I think my design should be more or less feasible to use for userspace. I didn't actually > >get as far as to write userspace code for it, but my idea for x11 was something along > >these lines: > > > >1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc > >2) if none was found pick an interpolated gamma mode with > > input/output bpc >= screen bpc > >3) fall back to whatever is left > > > >That approach should work for i915 + intel ddx at least. In theory it should be find for > >generic userspace too, but maybe it would need a few extra tweaks. > > > >Also we need fp16 to actually be able to test the interpolated modes fully. We now > >have that for icl, but I still need to post my "fp16 for everything gen4+" followup > >series. And after that we need to glue it all together in igt. > > > >Anyways, I've not worked on this branch myself in a while because I want to get the > >current gamma support fixed/cleaned up first. > >I have at least one more series after the current one gets reviewed. And after that we > >still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out. > > > >So yeah, I'd prefer to make the current stuff sensible before spending time adding > >fancier stuff on top. But in the meantime if you want to pick up where I left off with > >this gamma mode I'd be fine with that. > > Hi Ville, > Thanks for sharing these details and also the design what you have is really nice. > > I was trying to build on top of it and trying to check the interface before floating the next > version. There seems to be a limitation in creating a property with both BLOB and ENUM > flags. > > /* Only one legacy type at a time please */ > if (legacy_type && !is_power_of_2(legacy_type)) > return false; > > Any suggestion on how to deal with this limitation. I think we should just define a new BLOB_ENUM type. Trying to mix two old types like this could end in explosions with existing userspace so safer to make it a totally distinct type.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1736,6 +1736,7 @@ struct drm_i915_private { struct drm_property *force_audio_property; struct drm_property *gamma_mode_property; + struct drm_property *multi_segment_gamma_mode_property; /* hda/i915 audio component */ struct i915_audio_component *audio_component; diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 9d43d19..399d63d 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state drm_object_attach_property(&crtc->base.base, prop, 0); } +void +intel_attach_multi_segment_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->multi_segment_gamma_mode_property; + if (!prop) { + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, + "Multi-segment Gamma", 0); + if (!prop) + return; + + dev_priv->multi_segment_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. @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc) INTEL_INFO(dev_priv)->color.gamma_lut_size); intel_attach_gamma_mode_property(crtc); + + if (INTEL_GEN(dev_priv) >= 11) + intel_attach_multi_segment_gamma_mode_property(crtc); } diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info { __u8 data[]; }; +/* + * Structure for muti segmented gamma lut + */ +struct multi_segment_gamma_lut { + /* Number of Lut Segments */ + __u8 segment_cnt; + /* Precison of LUT entries in bits */ + __u8 precision_bits; + /* Pointer having number of LUT elements in each segment */ + __u32 *segment_lut_cnt_ptr; + /* Pointer to store exact lut values for each segment */ + __u32 *segment_lut_ptr; +}; + #if defined(__cplusplus) } #endif
Multi Segment Gamma Mode is added in Gen11+ platforms. Added a property interface to enable that. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++ include/uapi/drm/i915_drm.h | 14 ++++++++++++++ 3 files changed, 38 insertions(+)