| Message ID | 1554139811-13280-1-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Multi Segment Gamma Support | expand |
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. > > v2: Used Ville's design and approach to define the interfaces. > Addressed Matt Roper's review feedback and re-ordered the > patches. > > Uma Shankar (5): > drm: Add gamma mode property > drm/i915/icl: Add register definitions for Multi Segmented gamma > drm/i915/icl: Add support for multi segmented gamma mode > drm/i915: Add gamma mode caps property > drm/i915: Attach gamma mode property > > Ville Syrjälä (2): > drm: Add gamma mode caps property > drm/i915: Define color lut range structure > > drivers/gpu/drm/drm_atomic_uapi.c | 13 + > drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ > drivers/gpu/drm/i915/i915_reg.h | 17 ++ > drivers/gpu/drm/i915/intel_color.c | 465 ++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_display.c | 5 + > include/drm/drm_color_mgmt.h | 11 + > include/drm/drm_crtc.h | 17 ++ > include/drm/drm_mode_config.h | 10 + > include/uapi/drm/drm_mode.h | 49 ++++ > 9 files changed, 690 insertions(+), 7 deletions(-) > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville >Syrjälä >Sent: Friday, April 5, 2019 9:42 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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. 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. Regards, Uma Shankar >> >> v2: Used Ville's design and approach to define the interfaces. >> Addressed Matt Roper's review feedback and re-ordered the patches. >> >> Uma Shankar (5): >> drm: Add gamma mode property >> drm/i915/icl: Add register definitions for Multi Segmented gamma >> drm/i915/icl: Add support for multi segmented gamma mode >> drm/i915: Add gamma mode caps property >> drm/i915: Attach gamma mode property >> >> Ville Syrjälä (2): >> drm: Add gamma mode caps property >> drm/i915: Define color lut range structure >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ >> drivers/gpu/drm/i915/intel_color.c | 465 >++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_display.c | 5 + >> include/drm/drm_color_mgmt.h | 11 + >> include/drm/drm_crtc.h | 17 ++ >> include/drm/drm_mode_config.h | 10 + >> include/uapi/drm/drm_mode.h | 49 ++++ >> 9 files changed, 690 insertions(+), 7 deletions(-) >> >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville > >Syrjälä > >Sent: Friday, April 5, 2019 9:42 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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. > > Regards, > Uma Shankar > > >> > >> v2: Used Ville's design and approach to define the interfaces. > >> Addressed Matt Roper's review feedback and re-ordered the patches. > >> > >> Uma Shankar (5): > >> drm: Add gamma mode property > >> drm/i915/icl: Add register definitions for Multi Segmented gamma > >> drm/i915/icl: Add support for multi segmented gamma mode > >> drm/i915: Add gamma mode caps property > >> drm/i915: Attach gamma mode property > >> > >> Ville Syrjälä (2): > >> drm: Add gamma mode caps property > >> drm/i915: Define color lut range structure > >> > >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + > >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ > >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ > >> drivers/gpu/drm/i915/intel_color.c | 465 > >++++++++++++++++++++++++++++++++++- > >> drivers/gpu/drm/i915/intel_display.c | 5 + > >> include/drm/drm_color_mgmt.h | 11 + > >> include/drm/drm_crtc.h | 17 ++ > >> include/drm/drm_mode_config.h | 10 + > >> include/uapi/drm/drm_mode.h | 49 ++++ > >> 9 files changed, 690 insertions(+), 7 deletions(-) > >> > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >-- > >Ville Syrjälä > >Intel > >_______________________________________________ > >dri-devel mailing list > >dri-devel@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Monday, April 8, 2019 6:01 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@lists.freedesktop.org] On >> >Behalf Of Ville Syrjälä >> >Sent: Friday, April 5, 2019 9:42 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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 ? Note: I have currently used 16bit values only to get the feedback on the approach. Regards, Uma Shankar >> >> >> >> >> v2: Used Ville's design and approach to define the interfaces. >> >> Addressed Matt Roper's review feedback and re-ordered the patches. >> >> >> >> Uma Shankar (5): >> >> drm: Add gamma mode property >> >> drm/i915/icl: Add register definitions for Multi Segmented gamma >> >> drm/i915/icl: Add support for multi segmented gamma mode >> >> drm/i915: Add gamma mode caps property >> >> drm/i915: Attach gamma mode property >> >> >> >> Ville Syrjälä (2): >> >> drm: Add gamma mode caps property >> >> drm/i915: Define color lut range structure >> >> >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + >> >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ >> >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ >> >> drivers/gpu/drm/i915/intel_color.c | 465 >> >++++++++++++++++++++++++++++++++++- >> >> drivers/gpu/drm/i915/intel_display.c | 5 + >> >> include/drm/drm_color_mgmt.h | 11 + >> >> include/drm/drm_crtc.h | 17 ++ >> >> include/drm/drm_mode_config.h | 10 + >> >> include/uapi/drm/drm_mode.h | 49 ++++ >> >> 9 files changed, 690 insertions(+), 7 deletions(-) >> >> >> >> -- >> >> 1.9.1 >> >> >> >> _______________________________________________ >> >> Intel-gfx mailing list >> >> Intel-gfx@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> >-- >> >Ville Syrjälä >> >Intel >> >_______________________________________________ >> >dri-devel mailing list >> >dri-devel@lists.freedesktop.org >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel > >-- >Ville Syrjälä >Intel
On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Monday, April 8, 2019 6:01 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@lists.freedesktop.org] On > >> >Behalf Of Ville Syrjälä > >> >Sent: Friday, April 5, 2019 9:42 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> ><maarten.lankhorst@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. > > Note: I have currently used 16bit values only to get the feedback on the approach. > > Regards, > Uma Shankar > > >> > >> >> > >> >> v2: Used Ville's design and approach to define the interfaces. > >> >> Addressed Matt Roper's review feedback and re-ordered the patches. > >> >> > >> >> Uma Shankar (5): > >> >> drm: Add gamma mode property > >> >> drm/i915/icl: Add register definitions for Multi Segmented gamma > >> >> drm/i915/icl: Add support for multi segmented gamma mode > >> >> drm/i915: Add gamma mode caps property > >> >> drm/i915: Attach gamma mode property > >> >> > >> >> Ville Syrjälä (2): > >> >> drm: Add gamma mode caps property > >> >> drm/i915: Define color lut range structure > >> >> > >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + > >> >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ > >> >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ > >> >> drivers/gpu/drm/i915/intel_color.c | 465 > >> >++++++++++++++++++++++++++++++++++- > >> >> drivers/gpu/drm/i915/intel_display.c | 5 + > >> >> include/drm/drm_color_mgmt.h | 11 + > >> >> include/drm/drm_crtc.h | 17 ++ > >> >> include/drm/drm_mode_config.h | 10 + > >> >> include/uapi/drm/drm_mode.h | 49 ++++ > >> >> 9 files changed, 690 insertions(+), 7 deletions(-) > >> >> > >> >> -- > >> >> 1.9.1 > >> >> > >> >> _______________________________________________ > >> >> Intel-gfx mailing list > >> >> Intel-gfx@lists.freedesktop.org > >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > > >> >-- > >> >Ville Syrjälä > >> >Intel > >> >_______________________________________________ > >> >dri-devel mailing list > >> >dri-devel@lists.freedesktop.org > >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > >-- > >Ville Syrjälä > >Intel
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Monday, April 8, 2019 8:27 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >> >Sent: Monday, April 8, 2019 6:01 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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@lists.freedesktop.org] >> >> >On Behalf Of Ville Syrjälä >> >> >Sent: Friday, April 5, 2019 9:42 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> >> ><maarten.lankhorst@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 ? >> >> Note: I have currently used 16bit values only to get the feedback on the approach. >> >> Regards, >> Uma Shankar >> >> >> >> >> >> >> >> >> v2: Used Ville's design and approach to define the interfaces. >> >> >> Addressed Matt Roper's review feedback and re-ordered the patches. >> >> >> >> >> >> Uma Shankar (5): >> >> >> drm: Add gamma mode property >> >> >> drm/i915/icl: Add register definitions for Multi Segmented gamma >> >> >> drm/i915/icl: Add support for multi segmented gamma mode >> >> >> drm/i915: Add gamma mode caps property >> >> >> drm/i915: Attach gamma mode property >> >> >> >> >> >> Ville Syrjälä (2): >> >> >> drm: Add gamma mode caps property >> >> >> drm/i915: Define color lut range structure >> >> >> >> >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + >> >> >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ >> >> >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ >> >> >> drivers/gpu/drm/i915/intel_color.c | 465 >> >> >++++++++++++++++++++++++++++++++++- >> >> >> drivers/gpu/drm/i915/intel_display.c | 5 + >> >> >> include/drm/drm_color_mgmt.h | 11 + >> >> >> include/drm/drm_crtc.h | 17 ++ >> >> >> include/drm/drm_mode_config.h | 10 + >> >> >> include/uapi/drm/drm_mode.h | 49 ++++ >> >> >> 9 files changed, 690 insertions(+), 7 deletions(-) >> >> >> >> >> >> -- >> >> >> 1.9.1 >> >> >> >> >> >> _______________________________________________ >> >> >> Intel-gfx mailing list >> >> >> Intel-gfx@lists.freedesktop.org >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> > >> >> >-- >> >> >Ville Syrjälä >> >> >Intel >> >> >_______________________________________________ >> >> >dri-devel mailing list >> >> >dri-devel@lists.freedesktop.org >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel
On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Monday, April 8, 2019 8:27 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> >Sent: Monday, April 8, 2019 6:01 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> ><maarten.lankhorst@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@lists.freedesktop.org] > >> >> >On Behalf Of Ville Syrjälä > >> >> >Sent: Friday, April 5, 2019 9:42 PM > >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> ><maarten.lankhorst@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. > > >> > >> Note: I have currently used 16bit values only to get the feedback on the approach. > >> > >> Regards, > >> Uma Shankar > >> > >> >> > >> >> >> > >> >> >> v2: Used Ville's design and approach to define the interfaces. > >> >> >> Addressed Matt Roper's review feedback and re-ordered the patches. > >> >> >> > >> >> >> Uma Shankar (5): > >> >> >> drm: Add gamma mode property > >> >> >> drm/i915/icl: Add register definitions for Multi Segmented gamma > >> >> >> drm/i915/icl: Add support for multi segmented gamma mode > >> >> >> drm/i915: Add gamma mode caps property > >> >> >> drm/i915: Attach gamma mode property > >> >> >> > >> >> >> Ville Syrjälä (2): > >> >> >> drm: Add gamma mode caps property > >> >> >> drm/i915: Define color lut range structure > >> >> >> > >> >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + > >> >> >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ > >> >> >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ > >> >> >> drivers/gpu/drm/i915/intel_color.c | 465 > >> >> >++++++++++++++++++++++++++++++++++- > >> >> >> drivers/gpu/drm/i915/intel_display.c | 5 + > >> >> >> include/drm/drm_color_mgmt.h | 11 + > >> >> >> include/drm/drm_crtc.h | 17 ++ > >> >> >> include/drm/drm_mode_config.h | 10 + > >> >> >> include/uapi/drm/drm_mode.h | 49 ++++ > >> >> >> 9 files changed, 690 insertions(+), 7 deletions(-) > >> >> >> > >> >> >> -- > >> >> >> 1.9.1 > >> >> >> > >> >> >> _______________________________________________ > >> >> >> Intel-gfx mailing list > >> >> >> Intel-gfx@lists.freedesktop.org > >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> >> > > >> >> >-- > >> >> >Ville Syrjälä > >> >> >Intel > >> >> >_______________________________________________ > >> >> >dri-devel mailing list > >> >> >dri-devel@lists.freedesktop.org > >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel > >> > > >> >-- > >> >Ville Syrjälä > >> >Intel > > > >-- > >Ville Syrjälä > >Intel
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville >Syrjälä >Sent: Monday, April 8, 2019 9:15 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >> >Sent: Monday, April 8, 2019 8:27 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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@linux.intel.com] >> >> >Sent: Monday, April 8, 2019 6:01 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> >> ><maarten.lankhorst@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@lists.freedesktop.org] >> >> >> >On Behalf Of Ville Syrjälä >> >> >> >Sent: Friday, April 5, 2019 9:42 PM >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> >> ><maarten.lankhorst@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 ? >> >> >> >> >> Note: I have currently used 16bit values only to get the feedback on the >approach. >> >> >> >> Regards, >> >> Uma Shankar >> >> >> >> >> >> >> >> >> >> >> >> >> v2: Used Ville's design and approach to define the interfaces. >> >> >> >> Addressed Matt Roper's review feedback and re-ordered the patches. >> >> >> >> >> >> >> >> Uma Shankar (5): >> >> >> >> drm: Add gamma mode property >> >> >> >> drm/i915/icl: Add register definitions for Multi Segmented gamma >> >> >> >> drm/i915/icl: Add support for multi segmented gamma mode >> >> >> >> drm/i915: Add gamma mode caps property >> >> >> >> drm/i915: Attach gamma mode property >> >> >> >> >> >> >> >> Ville Syrjälä (2): >> >> >> >> drm: Add gamma mode caps property >> >> >> >> drm/i915: Define color lut range structure >> >> >> >> >> >> >> >> drivers/gpu/drm/drm_atomic_uapi.c | 13 + >> >> >> >> drivers/gpu/drm/drm_color_mgmt.c | 110 +++++++++ >> >> >> >> drivers/gpu/drm/i915/i915_reg.h | 17 ++ >> >> >> >> drivers/gpu/drm/i915/intel_color.c | 465 >> >> >> >++++++++++++++++++++++++++++++++++- >> >> >> >> drivers/gpu/drm/i915/intel_display.c | 5 + >> >> >> >> include/drm/drm_color_mgmt.h | 11 + >> >> >> >> include/drm/drm_crtc.h | 17 ++ >> >> >> >> include/drm/drm_mode_config.h | 10 + >> >> >> >> include/uapi/drm/drm_mode.h | 49 ++++ >> >> >> >> 9 files changed, 690 insertions(+), 7 deletions(-) >> >> >> >> >> >> >> >> -- >> >> >> >> 1.9.1 >> >> >> >> >> >> >> >> _______________________________________________ >> >> >> >> Intel-gfx mailing list >> >> >> >> Intel-gfx@lists.freedesktop.org >> >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> > >> >> >> >-- >> >> >> >Ville Syrjälä >> >> >> >Intel >> >> >> >_______________________________________________ >> >> >> >dri-devel mailing list >> >> >> >dri-devel@lists.freedesktop.org >> >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> > >> >> >-- >> >> >Ville Syrjälä >> >> >Intel >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville > >Syrjälä > >Sent: Monday, April 8, 2019 9:15 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> >Sent: Monday, April 8, 2019 8:27 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> ><maarten.lankhorst@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@linux.intel.com] > >> >> >Sent: Monday, April 8, 2019 6:01 PM > >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> ><maarten.lankhorst@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@lists.freedesktop.org] > >> >> >> >On Behalf Of Ville Syrjälä > >> >> >> >Sent: Friday, April 5, 2019 9:42 PM > >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; > >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; > >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> >> ><maarten.lankhorst@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.
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville >Syrjälä >Sent: Monday, April 8, 2019 9:38 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@lists.freedesktop.org] On >> >Behalf Of Ville Syrjälä >> >Sent: Monday, April 8, 2019 9:15 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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@linux.intel.com] >> >> >Sent: Monday, April 8, 2019 8:27 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> >> ><maarten.lankhorst@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@linux.intel.com] >> >> >> >Sent: Monday, April 8, 2019 6:01 PM >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> >> ><maarten.lankhorst@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@lists.freedesktop.org] >> >> >> >> >On Behalf Of Ville Syrjälä >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> >> >> ><maarten.lankhorst@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. In both cases, we will use the existing GAMMA_LUT blob property to send the lut values from userspace. Will send out the next version soon. Thanks for your valuable feedback and suggestions. Regards, Uma Shankar > >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Apr 10, 2019 at 01:20:44PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville > >Syrjälä > >Sent: Monday, April 8, 2019 9:38 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@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@lists.freedesktop.org] On > >> >Behalf Of Ville Syrjälä > >> >Sent: Monday, April 8, 2019 9:15 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> ><maarten.lankhorst@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@linux.intel.com] > >> >> >Sent: Monday, April 8, 2019 8:27 PM > >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- > >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville > >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> ><maarten.lankhorst@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@linux.intel.com] > >> >> >> >Sent: Monday, April 8, 2019 6:01 PM > >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; > >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; > >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> >> ><maarten.lankhorst@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@lists.freedesktop.org] > >> >> >> >> >On Behalf Of Ville Syrjälä > >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM > >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; > >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; > >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> >> >> ><maarten.lankhorst@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.
>> >> >-----Original Message----- >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On >> >Behalf Of Ville Syrjälä >> >Sent: Monday, April 8, 2019 9:38 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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@lists.freedesktop.org] >> >> >On Behalf Of Ville Syrjälä >> >> >Sent: Monday, April 8, 2019 9:15 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten >> >> ><maarten.lankhorst@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@linux.intel.com] >> >> >> >Sent: Monday, April 8, 2019 8:27 PM >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> >> ><maarten.lankhorst@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@linux.intel.com] >> >> >> >> >Sent: Monday, April 8, 2019 6:01 PM >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> >> >> ><maarten.lankhorst@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@lists.freedesktop.org] >> >> >> >> >> >On Behalf Of Ville Syrjälä >> >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM >> >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >> >> >> >Cc: dcastagna@chromium.org; >> >> >> >> >> >intel-gfx@lists.freedesktop.org; >> >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org; >> >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, >> >> >> >> >> >Maarten <maarten.lankhorst@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. If user sets the Client Cap then he will ignore the DE)GAMMA_LUT_SIZE properties and will rely on the new property interface. Data passed as blob in already available GAMMA_LUT property will be taken in conjunction with the mode set by new GAMMA_MODE property . Now if this app finishes and new user comes up, and if it doesn't sets the Client CAP that means its not planning to use new interface, as part of gamma_lut programming we will fallback to linear lut with default mode. This anyways would have been the state if the first app was not there. And if the new app wants to use legacy method to change (DE)GAMMA lut he can get the default size in LUT_SIZE property which will still be good for legacy and work with that. I feel this will solve the issues we may get and both can co-exist. Regards, Uma Shankar >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel