>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 2:50 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten
><maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Sharma,
>Shashank <shashank.sharma@xxxxxxxxx>
>Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
>
>On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
>> Gen platforms support multiple gamma modes, currently it's hard coded
>> to operate only in 1 specific mode.
>> This patch adds a property to make gamma mode programmable.
>> User can select which mode should be used for a particular usecase or
>> scenario.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>
>I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
>approach using this property. This seems to be exposing hardware implementation
>details that I wouldn't expect userspace to need to worry about (plus I don't think any
>of the property values here convey any specific meaning to someone who hasn't read
>the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
>driver takes care of the programming details and userspace never sees the actual way
>the registers are laid out and written?
>My understanding is that what really matters is how many table entries there are for
>userspace to fill in, what input range(s) they cover, and how the bits of each table
>entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
>DRM core if possible; most of the display servers that get used these days are cross-
>platform and probably won't want to add Intel-specific logic (or platform-specific
>logic if we wind up with a different set of options on future Intel platforms).
Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
documentation for the usage of the property.
@Ville- What do you recommend or suggest for these interfaces.
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_color.c | 46
>++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 3 +++
>> 3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index c65c2e6..02231ae 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>> struct drm_property *broadcast_rgb_property;
>> struct drm_property *force_audio_property;
>>
>> + struct drm_property *gamma_mode_property;
>> +
>> /* hda/i915 audio component */
>> struct i915_audio_component *audio_component;
>> bool audio_component_registered;
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 467fd1a..9d43d19 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -92,6 +92,19 @@
>> 0x0800, 0x0100, 0x0800,
>> };
>>
>> +#define LEGACY_PALETTE_MODE_8BIT BIT(0)
>> +#define PRECISION_PALETTE_MODE_10BIT BIT(1)
>> +#define INTERPOLATED_GAMMA_MODE_12BIT BIT(2)
>> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT BIT(3)
>> +#define SPLIT_GAMMA_MODE_12BIT BIT(4)
>> +
>> +#define INTEL_GAMMA_MODE_MASK (\
>> + LEGACY_PALETTE_MODE_8BIT | \
>> + PRECISION_PALETTE_MODE_10BIT | \
>> + INTERPOLATED_GAMMA_MODE_12BIT | \
>> + MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
>> + BIT_SPLIT_GAMMA_MODE_12BIT)
>
>Is the "BIT_" prefix on this last one a typo? I assume this was supposed to just be the
>SPLIT_GAMMA_MODE_12BIT defined above?
Yes, will fix this.
>> +
>> static bool lut_is_legacy(const struct drm_property_blob *lut) {
>> return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -105,6
>> +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state
>*crtc_state
>> lut_is_legacy(crtc_state->base.gamma_lut);
>> }
>>
>> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
>> + { LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
>> + { PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
>> + { INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma
>Mode" },
>> + { MULTI_SEGMENTED_GAMMA_MODE_12BIT,
>> + "12 Bit Multi Segmented Gamma Mode" },
>> + { SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" }, };
>> +
>> +void
>> +intel_attach_gamma_mode_property(struct intel_crtc *crtc) {
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_property *prop;
>> +
>> + prop = dev_priv->gamma_mode_property;
>> + if (!prop) {
>> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> + "Gamma Mode",
>> + gamma_mode_supported,
>> +
> ARRAY_SIZE(gamma_mode_supported));
>
>If we do expose hardware-specific gamma modes like this, then I think we'd want to
>create this property with a platform-specific list of modes so that userspace doesn't
>even have the options for modes that aren't supported on the platform they're
>running on.
Ok, will add the property enum based on platform capabilities.
>> + if (!prop)
>> + return;
>> +
>> + dev_priv->gamma_mode_property = prop;
>> + }
>> +
>> + drm_object_attach_property(&crtc->base.base, prop, 0); }
>> +
>> /*
>> * When using limited range, multiply the matrix given by userspace by
>> * the matrix that we would use for the limited range.
>> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>> INTEL_INFO(dev_priv)-
>>color.degamma_lut_size,
>> true,
>> INTEL_INFO(dev_priv)-
>>color.gamma_lut_size);
>> +
>> + intel_attach_gamma_mode_property(crtc);
>
>It looks like we're exposing the property to userspace in this patch, but we don't finish
>wiring up the functionality until later patches in the series; that's going to make things
>confusing if someone bisects over this range of patches. It would be best to hold off
>on exposing new interfaces like this to userspace until the end of the implementation
>when they're fully functional.
Ok, will move the attach to a later patch when all the necessary ingredients are in place.
Regards,
Uma Shankar
>
>Matt
>
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d9f188e..fd84fe9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>> u8 nv12_planes;
>> u8 c8_planes;
>>
>> + /* Gamma mode type programmed on the pipe */
>> + u32 gamma_mode_type;
>> +
>> /* bitmask of planes that will be updated during the commit */
>> u8 update_planes;
>>
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
