On Thu, Mar 28, 2019 at 07:00:30PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >Sent: Friday, March 22, 2019 7:23 PM > >To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten > ><maarten.lankhorst@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >Subject: Re: [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@xxxxxxxxx> > >> >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; > >> >intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Sharma, Shashank > >> ><shashank.sharma@xxxxxxxxx>; Roper, Matthew D > >> ><matthew.d.roper@xxxxxxxxx> > >> >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@xxxxxxxxx> > >> >> > --- > >> >> > 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. -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
