>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Monday, April 8, 2019 8:27 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: dcastagna@xxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- >devel@xxxxxxxxxxxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; Syrjala, Ville ><ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >Subject: Re: [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@xxxxxxxxxxxxxxx] >> >Sent: Monday, April 8, 2019 6:01 PM >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >Cc: dcastagna@xxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- >> >devel@xxxxxxxxxxxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; Syrjala, Ville >> ><ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten >> ><maarten.lankhorst@xxxxxxxxx> >> >Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx] >> >> >On Behalf Of Ville Syrjälä >> >> >Sent: Friday, April 5, 2019 9:42 PM >> >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >> >Cc: dcastagna@xxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- >> >> >devel@xxxxxxxxxxxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; Syrjala, Ville >> >> ><ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten >> >> ><maarten.lankhorst@xxxxxxxxx> >> >> >Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> > >> >> >-- >> >> >Ville Syrjälä >> >> >Intel >> >> >_______________________________________________ >> >> >dri-devel mailing list >> >> >dri-devel@xxxxxxxxxxxxxxxxxxxxx >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
