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

[Shankar, Uma]

>-----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 ?

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 at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>> >_______________________________________________
>> >dri-devel mailing list
>> >dri-devel at lists.freedesktop.org
>> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel