>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Friday, March 22, 2019 7:23 PM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten
><maarten.lankho...@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.lankho...@intel.com>
>> >Cc: Shankar, Uma <uma.shan...@intel.com>;
>> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
>> ><shashank.sha...@intel.com>; Roper, Matthew D
>> ><matthew.d.ro...@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.shan...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
