Re: [v2 0/7] Add Multi Segment Gamma Support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux