[Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

Wed Apr 10 15:38:27 UTC 2019

On Wed, Apr 10, 2019 at 01:20:44PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Monday, April 8, 2019 9:38 PM
> >To: Shankar, Uma <uma.shankar at intel.com>
> >Cc: dcastagna at chromium.org; intel-gfx at lists.freedesktop.org; dri-
> >devel at lists.freedesktop.org; seanpaul at chromium.org; Syrjala, Ville
> ><ville.syrjala at intel.com>; Lankhorst, Maarten <maarten.lankhorst at intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On
> >> >Behalf Of Ville Syrjälä
> >> >Sent: Monday, April 8, 2019 9:15 PM
> >> >To: Shankar, Uma <uma.shankar at intel.com>
> >> >Cc: dcastagna at chromium.org; intel-gfx at lists.freedesktop.org; dri-
> >> >devel at lists.freedesktop.org; seanpaul at chromium.org; Syrjala, Ville
> >> ><ville.syrjala at intel.com>; Lankhorst, Maarten
> >> ><maarten.lankhorst at intel.com>
> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >
> >> >On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
> >> >>
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> >> >> >Sent: Monday, April 8, 2019 8:27 PM
> >> >> >To: Shankar, Uma <uma.shankar at intel.com>
> >> >> >Cc: dcastagna at chromium.org; intel-gfx at lists.freedesktop.org; dri-
> >> >> >devel at lists.freedesktop.org; seanpaul at chromium.org; Syrjala, Ville
> >> >> ><ville.syrjala at intel.com>; Lankhorst, Maarten
> >> >> ><maarten.lankhorst at intel.com>
> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >> >
> >> >> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
> >> >> >>
> >> >> >>
> >> >> >> >-----Original Message-----
> >> >> >> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> >> >> >> >Sent: Monday, April 8, 2019 6:01 PM
> >> >> >> >To: Shankar, Uma <uma.shankar at intel.com>
> >> >> >> >Cc: dcastagna at chromium.org; intel-gfx at lists.freedesktop.org;
> >> >> >> >dri- devel at lists.freedesktop.org; seanpaul at chromium.org;
> >> >> >> >Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
> >> >> >> ><maarten.lankhorst at intel.com>
> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
> >> >> >> >Support
> >> >> >> >
> >> >> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> >-----Original Message-----
> >> >> >> >> >From: dri-devel
> >> >> >> >> >[mailto:dri-devel-bounces at lists.freedesktop.org]
> >> >> >> >> >On Behalf Of Ville Syrjälä
> >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM
> >> >> >> >> >To: Shankar, Uma <uma.shankar at intel.com>
> >> >> >> >> >Cc: dcastagna at chromium.org; intel-gfx at lists.freedesktop.org;
> >> >> >> >> >dri- devel at lists.freedesktop.org; seanpaul at chromium.org;
> >> >> >> >> >Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
> >> >> >> >> ><maarten.lankhorst at intel.com>
> >> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
> >> >> >> >> >Support
> >> >> >> >> >
> >> >> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> >> >> >> >> This series adds support for programmable gamma modes and
> >> >> >> >> >> exposes a property interface for the same. Also added,
> >> >> >> >> >> support for multi segment gamma mode introduced in ICL+
> >> >> >> >> >>
> >> >> >> >> >> It creates 2 property interfaces :
> >> >> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
> >> >> >> >> >> the various gamma modes supported and the lut ranges. This
> >> >> >> >> >> is an enum property with element as blob id. Getting the
> >> >> >> >> >> blob id in userspace, user can get the mode supported and
> >> >> >> >> >> also the range of gamma mode supported with number of lut
> >coefficients.
> >> >> >> >> >>
> >> >> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and
> >> >> >> >> >> send the lut values for that particular mode.
> >> >> >> >> >
> >> >> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
> >> >> >> >> >Then the possible values and the current value are all part of the same
> >prop.
> >> >> >> >>
> >> >> >> >> Hi Ville,
> >> >> >> >> With the current approach, we have enum property with values
> >> >> >> >> as blob_ids (representing platform capabilities). This should
> >> >> >> >> not get modified and needs to be immutable.
> >> >> >> >
> >> >> >> >That's not quite what we want. We want to let the user modify
> >> >> >> >the current value so that they can actually select the gamma mode.
> >> >> >> >Otherwise we need yet another prop for it, or we have to deduce
> >> >> >> >it from the LUT size (that apporach would actually work for
> >> >> >> >i915 but may not work for other drivers/hardware).
> >> >> >> >
> >> >> >> >>
> >> >> >> >> Userspace can query the property and get the blob using the blob_ids.
> >> >> >> >> Thereby getting all the platform capabilities.
> >> >> >> >>
> >> >> >> >> Now to set the LUT values, he can use another blob property
> >> >> >> >> and pass the luts.  This is inline to how gamma/degamma is
> >> >> >> >> implemented where we have one immutable LUT_SIZE property
> >> >> >> >> (indicating number of
> >> >> >> >> luts) and another blob property for actual lut values..
> >> >> >>
> >> >> >> Hi Ville,
> >> >> >> Just to clarify and clear some doubts :)
> >> >> >>
> >> >> >> We should be able to set the gamma mode using the blob enum value.
> >> >> >> Userspace will get the list enum vals (blob ids with mode name
> >> >> >> embedded) and
> >> >> >select one and do a setprop to set a mode.
> >> >> >> Driver will get the blob_id and will be able to get the mode to
> >> >> >> be set.  So exposing capabilities and setting the mode should be
> >> >> >> possible with this one
> >> >> >property. I hope my understanding is correct.
> >> >> >>
> >> >> >> Now to send the actual blob values to be set, we need to use
> >> >> >> some other property interface. Should we use the currently
> >> >> >> available "gamma blob
> >> >> >(gamma_lut_property)" property to send the lut values.
> >> >> >> The challenge there is that it currently uses 16 bit lut values
> >> >> >> struct drm_color_lut {
> >> >> >>         __u16 red;
> >> >> >>         __u16 green;
> >> >> >>         __u16 blue;
> >> >> >>         __u16 reserved;
> >> >> >> };
> >> >> >> which is not sufficient for HDR usecases. Or should we need a
> >> >> >> new property for
> >> >> >advance lut/extended lut like below:
> >> >> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev
> >> >> >> =7
> >> >> >>
> >> >> >> What do you suggest ?
> >> >> >
> >> >> >I was thinking that we might get away with reusing the current
> >> >> >props and just change the interpretation of the data when
> >> >> >gamma_mode is set. But I'm not sure that's going to work out so
> >> >> >well if one client sets this up and then another client comes
> >> >> >along that doesn't understand the new props at all. But even with
> >> >> >separate props I think we might still end up in a mess because the
> >> >> >new client wouldn't know how to unset the higher precision LUT
> >> >> >before setting up the old style prop and the
> >> >kernel would then refuse the operation with with both props being set.
> >> >> >
> >> >> >So I think we might need a client cap for this which simply
> >> >> >changes how the data in the existing props is represented. So
> >> >> >internally we could always store things in the new high precision
> >> >> >format, but we'd convert to/from the old format when dealing with an older
> >client.
> >> >>
> >> >> We could also say that if a legacy gamma_mode_property is set
> >> >> (which will be used by legacy apps or apps not aware of new
> >> >> interface), in driver we will simply unset the earlier gamma_mode
> >> >> and fallback to legacy mode (whatever it was for a particular
> >> >> platform). This way we should be able to deal with this situation and an explicit
> >unset may not be needed.
> >> >What do you think ?
> >> >
> >> >I don't want (non-immutable) properties that magically change values.
> >> >That way lies madness.
> >>
> >> Oh ok. So do you suggest that we add some kind of  flag to be set by
> >> user, based on which we take either legacy or advance_gamma path. Is my
> >understanding correct ?
> >
> >Yeah, just another client cap. I can't immediately think of a nicer way to extend the
> >precision.
> 
> Sure Ville, I am implementing based on this suggestion.  Basically will expose a new 
> advance_gamma_mode flag as a client cap. Something like:
> 
> #define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES     6
> 
> The new users will set this and we will assume that advance gamma mode paths is active.
> If the flag is not set, driver will take the legacy path and enable default gamma_mode for
> that particular platform.

It's maybe a still a bit nasty because we basically have to ignore the
gamma mode prop entirely in that case. Not quite sure what is the best
way to shoehorn this in. I guess the main problem is what we would
report to userspace via the (DE)GAMMA_MODE props if the previous client
left gamma_mode set in a way that conflicts with what
(DE)GAMMA_LUT_SIZE suggests it should be.

-- 
Ville Syrjälä
Intel