Re: [PATCH 00/11] drm/i915: adding state checker for gamma lut values

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 17 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Thanks to Jani N, Matt and Ville for the review comments. Hopefully
> I have addressed all the current review comments and ready to receive
> more :)

Sorry to take for so long to notice... but all of the patches need a
proper commit message. It doesn't have to be long if the change is
simple, but there has to be more than just the changelog.

BR,
Jani.



>
> In this patch series, added state checker to validate gamma_lut values. This
> reads hardware state, and compares the originally requested
> state to the state read from hardware.
>
> v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani)
>     -Added inverse function of drm_color_lut_extract to convert hardware
>      read values back to user values (code written by Jani)
>     -Renamed get_config() to color_config() (Jani)
>     -Placed all platform specific shifts and masks in i915_reg.h (Jani)
>     -Renamed i9xx_get_config to i9xx_color_config and all related
>      functions (Jani)
>     -Removed debug logs from compare function (Jani)
>     -Renamed intel_compare_blob to intel_compare_lut and added platform specific
>      bit precision of the readout into the function (Jani)
>     -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani)
>     -Added check if blobs can be NULL (Jani)
>     -Added function in intel_color.c that returns the bit precision (Jani),
>      didn't add in device info since its gonna die soon (Ville)
>
> v2: -Moved intel_compare_lut() from intel_display.c to intel_color.c (Jani)
>     -Changed color_config() func names to get_color_config() and same
>      for gamma func too (Jani)
>     -Removed blank line in i915_reg.h and aligned MASK with their
>      register name (Jani)
>     -Mask definition code indented and defined using REG_GENMASK() and
>      used using REG_FIELD_GET() (Jani)
>     -Made bit_precision func inline (Jani/Matt)
>     -Assigned bit_precision according to GAMMA_MODE (Matt/Ville)
>     -Changed IS_ERR(blob) condition to (!blob) (Jani)
>     -Rearranged blob check and lut_size conditions (Jani)
>     -Used inline function for the comparison (Jani)
>     -Separated the get config part from the state checker part to
>      another patch (Jani)
>     -Retained "internal" i9xx_internal_gamma_config for consistency
>      (Matt)
>     -Removed crtc_state_is_legacy_gamma check and replaced with
>      GAMMA_MODE (Matt)
>     -Created whole platform specific patch series as submitted by Ville for
>      clean up intel_color_check()
>     -Rebased on top of Ville's changes
>
> v3: -Rebase
>
> v4: -Renamed intel_get_color_config to intel_color_get_config (Jani)
>     -Wrapped get_color_config() (Jani)
>     -Added the user early on such that support for get_color_config()
>      can be added platform by platform incrementally (Jani)
>     -Few changes in get_config_func (Jani)
>     -Renamed intel_compare_color_lut() to intel_color_lut_equal() (Jani)
>     -Corrected smatch warn "variable dereferenced before check" (Dan Carpenter)
>
> TODO:  -Matt suggested to rename get_gamma_config to get_gamma_lut or
>         i9xx_read_out_gamma_lut, Ville also named functions like i9xx_read_lut_8(),
>         i9xx_read_lut_10(). Should we follow this?
>        -Answering Matt's query regarding intension to read and compare degamma/ctm".
>         Degamma will be my first priority after this and later ctm.
>         Should we have combined func or separate func for gamma/degamma val?
>        -Add separate func to log errors. I am not sure, what needs to be printed here?
>         Mismatched RGB value? 
>        -Will make changes in next rev for changes made by Ville for cherryview_load_luts()
>
>
> Swati Sharma (11):
>   [v4] drm/i915: Introduce vfunc intel_get_color_config to create hw lut
>   [v4] drm/i915: Enable intel_color_get_config()
>   [v4] drm/i915: Extract i9xx_get_color_config()
>   [v4] drm/i915: Extract cherryview_get_color_config()
>   [v4] drm/i915: Extract i965_get_color_config()
>   [v4] drm/i915: Extract icl_get_color_config()
>   [v4] drm/i915: Extract glk_get_color_config()
>   [v4] drm/i915: Extract bdw_get_color_config()
>   [v4] drm/i915: Extract ivb_get_color_config()
>   [v4] drm/i915: Extract ilk_get_color_config()
>   [v4] drm/i915: Add intel_color_lut_equal() to compare hw and sw gamma
>     lut values
>
>  drivers/gpu/drm/i915/i915_drv.h      |   1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  15 ++
>  drivers/gpu/drm/i915/intel_color.c   | 343 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_color.h   |   8 +
>  drivers/gpu/drm/i915/intel_display.c |  13 ++
>  5 files changed, 375 insertions(+), 5 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx