Re: [Intel-gfx] [PATCH 4/5] drm/i915: Implement color management on bdw/skl/bxt/kbl

2016-02-25 Thread Matt Roper 
On Thu, Feb 25, 2016 at 03:33:42PM +, Lionel Landwerlin wrote:
> Patch based on a previous series by Shashank Sharma.
> 
> v2: Do not read GAMMA_MODE register to figure what mode we're in
> 
> v3: Program PREC_PAL_GC_MAX to clamp pixel values > 1.0
> 
> Add documentation on how the Broadcast RGB property is affected by CTM
> 
> v4: Update contributors
> 
> v5: Refactor degamma/gamma LUTs load into a single function
> 
> v6: Fix missing intel_crtc variable (bisect issue)
> 
> v7: Fix & simplify limited range matrix multiplication
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Lionel Landwerlin 
> Signed-off-by: Kumar, Kiran S 
> Signed-off-by: Kausal Malladi 

There are two trivial typo/style items that I call out farther down, but
aside from those, you've addressed everything I saw in earlier versions
of this patch.  It's a pretty large/complicated patch overall, so there
may be things I missed, but I think it's in good enough shape to merge.

Acknowledged-by: Matt Roper 


> ---
>  Documentation/DocBook/gpu.tmpl   |   6 +-
>  drivers/gpu/drm/i915/i915_drv.c  |  24 ++-
>  drivers/gpu/drm/i915/i915_drv.h  |   6 +
>  drivers/gpu/drm/i915/i915_reg.h  |  22 +++
>  drivers/gpu/drm/i915/intel_color.c   | 341 
> ++-
>  drivers/gpu/drm/i915/intel_display.c |  22 ++-
>  drivers/gpu/drm/i915/intel_drv.h |   3 +-
>  drivers/gpu/drm/i915/intel_fbdev.c   |   8 +
>  8 files changed, 369 insertions(+), 63 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1692c4d..430e99b 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2153,7 +2153,11 @@ void intel_crt_init(struct drm_device *dev)
>   ENUM
>   { "Automatic", "Full", "Limited 16:235" }
>   Connector
> - TBD
> + When this property is set to Limited 16:235
> + and CTM is set, the hardware will be programmed with the
> + result of the multiplication of CTM by the limited range
> + matrix to ensure the pixels normaly in the range 0..1.0 are
> + remapped to the range 16/255..235/255.
>   
>   
>   “audio”
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 20e8200..3807b73 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -66,6 +66,9 @@ static struct drm_driver driver;
>  #define IVB_CURSOR_OFFSETS \
>   .cursor_offsets = { CURSOR_A_OFFSET, IVB_CURSOR_B_OFFSET, 
> IVB_CURSOR_C_OFFSET }
>  
> +#define BDW_COLORS \
> + .color = { .degamma_lut_size = 512, .gamma_lut_size = 512 }
> +
>  static const struct intel_device_info intel_i830_info = {
>   .gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
>   .has_overlay = 1, .overlay_needs_physical = 1,
> @@ -288,24 +291,28 @@ static const struct intel_device_info 
> intel_haswell_m_info = {
>   .is_mobile = 1,
>  };
>  
> +#define BDW_FEATURES \
> + HSW_FEATURES, \
> + BDW_COLORS
> +
>  static const struct intel_device_info intel_broadwell_d_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8,
>  };
>  
>  static const struct intel_device_info intel_broadwell_m_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8, .is_mobile = 1,
>  };
>  
>  static const struct intel_device_info intel_broadwell_gt3d_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  };
>  
>  static const struct intel_device_info intel_broadwell_gt3m_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8, .is_mobile = 1,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  };
> @@ -321,13 +328,13 @@ static const struct intel_device_info 
> intel_cherryview_info = {
>  };
>  
>  static const struct intel_device_info intel_skylake_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_skylake = 1,
>   .gen = 9,
>  };
>  
>  static const struct intel_device_info intel_skylake_gt3_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_skylake = 1,
>   .gen = 9,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> @@ -345,17 +352,18 @@ static const struct intel_device_info 
> intel_broxton_info = {
>   .has_fbc = 1,
>   GEN_DEFAULT_PIPEOFFSETS,
>   IVB_CURSOR_OFFSETS,
> + BDW_COLORS,
>  };
>  
>  static const struct intel_device_info intel_kabylake_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_preliminary = 1,
>   .is_kabylake = 1,
>   .gen = 9,
>  };
>  
>  static const struct intel_device_info intel_kabylake_gt3_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_preliminary = 1,
>

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Implement color management on bdw/skl/bxt/kbl

2016-02-23 Thread Lionel Landwerlin 

On 23/02/16 00:52, Matt Roper wrote:

On Mon, Feb 22, 2016 at 02:18:10PM +, Lionel Landwerlin wrote:

Patch based on a previous series by Shashank Sharma.

v2: Do not read GAMMA_MODE register to figure what mode we're in

v3: Program PREC_PAL_GC_MAX to clamp pixel values > 1.0

 Add documentation on how the Broadcast RGB property is affected by CTM

v4: Update contributors

v5: Refactor degamma/gamma LUTs load into a single function

v6: Fix missing intel_crtc variable (bisect issue)

Not sure if you saw me feedback on v4 or not:
https://lists.freedesktop.org/archives/intel-gfx/2016-February/088043.html

It looks like most of those comments still apply here.  If you disagree
with my comments, that's fine too, I just wanted to make sure it didn't
get overlooked.  :-)


Matt


Sorry for that, completely missed on the first 3/4 comments.
You're right the limited range multiplication was wrong (input[i] vs 
result[i]...).

Fixed that and simplified it as you suggested.

Thanks a lot!

-
Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Implement color management on bdw/skl/bxt/kbl

2016-02-22 Thread Matt Roper 
On Mon, Feb 22, 2016 at 02:18:10PM +, Lionel Landwerlin wrote:
> Patch based on a previous series by Shashank Sharma.
> 
> v2: Do not read GAMMA_MODE register to figure what mode we're in
> 
> v3: Program PREC_PAL_GC_MAX to clamp pixel values > 1.0
> 
> Add documentation on how the Broadcast RGB property is affected by CTM
> 
> v4: Update contributors
> 
> v5: Refactor degamma/gamma LUTs load into a single function
> 
> v6: Fix missing intel_crtc variable (bisect issue)

Not sure if you saw me feedback on v4 or not:
   https://lists.freedesktop.org/archives/intel-gfx/2016-February/088043.html

It looks like most of those comments still apply here.  If you disagree
with my comments, that's fine too, I just wanted to make sure it didn't
get overlooked.  :-)


Matt

> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Lionel Landwerlin 
> Signed-off-by: Kumar, Kiran S 
> Signed-off-by: Kausal Malladi 
> ---
>  Documentation/DocBook/gpu.tmpl   |   6 +-
>  drivers/gpu/drm/i915/i915_drv.c  |  24 ++-
>  drivers/gpu/drm/i915/i915_drv.h  |   6 +
>  drivers/gpu/drm/i915/i915_reg.h  |  22 +++
>  drivers/gpu/drm/i915/intel_color.c   | 356 
> ++-
>  drivers/gpu/drm/i915/intel_display.c |  22 ++-
>  drivers/gpu/drm/i915/intel_drv.h |   3 +-
>  drivers/gpu/drm/i915/intel_fbdev.c   |   8 +
>  8 files changed, 383 insertions(+), 64 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1692c4d..430e99b 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2153,7 +2153,11 @@ void intel_crt_init(struct drm_device *dev)
>   ENUM
>   { "Automatic", "Full", "Limited 16:235" }
>   Connector
> - TBD
> + When this property is set to Limited 16:235
> + and CTM is set, the hardware will be programmed with the
> + result of the multiplication of CTM by the limited range
> + matrix to ensure the pixels normaly in the range 0..1.0 are
> + remapped to the range 16/255..235/255.
>   
>   
>   “audio”
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 20e8200..3807b73 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -66,6 +66,9 @@ static struct drm_driver driver;
>  #define IVB_CURSOR_OFFSETS \
>   .cursor_offsets = { CURSOR_A_OFFSET, IVB_CURSOR_B_OFFSET, 
> IVB_CURSOR_C_OFFSET }
>  
> +#define BDW_COLORS \
> + .color = { .degamma_lut_size = 512, .gamma_lut_size = 512 }
> +
>  static const struct intel_device_info intel_i830_info = {
>   .gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
>   .has_overlay = 1, .overlay_needs_physical = 1,
> @@ -288,24 +291,28 @@ static const struct intel_device_info 
> intel_haswell_m_info = {
>   .is_mobile = 1,
>  };
>  
> +#define BDW_FEATURES \
> + HSW_FEATURES, \
> + BDW_COLORS
> +
>  static const struct intel_device_info intel_broadwell_d_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8,
>  };
>  
>  static const struct intel_device_info intel_broadwell_m_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8, .is_mobile = 1,
>  };
>  
>  static const struct intel_device_info intel_broadwell_gt3d_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  };
>  
>  static const struct intel_device_info intel_broadwell_gt3m_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .gen = 8, .is_mobile = 1,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  };
> @@ -321,13 +328,13 @@ static const struct intel_device_info 
> intel_cherryview_info = {
>  };
>  
>  static const struct intel_device_info intel_skylake_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_skylake = 1,
>   .gen = 9,
>  };
>  
>  static const struct intel_device_info intel_skylake_gt3_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_skylake = 1,
>   .gen = 9,
>   .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> @@ -345,17 +352,18 @@ static const struct intel_device_info 
> intel_broxton_info = {
>   .has_fbc = 1,
>   GEN_DEFAULT_PIPEOFFSETS,
>   IVB_CURSOR_OFFSETS,
> + BDW_COLORS,
>  };
>  
>  static const struct intel_device_info intel_kabylake_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_preliminary = 1,
>   .is_kabylake = 1,
>   .gen = 9,
>  };
>  
>  static const struct intel_device_info intel_kabylake_gt3_info = {
> - HSW_FEATURES,
> + BDW_FEATURES,
>   .is_preliminary = 1,
>   .is_kabylake = 1,
>   .gen = 9,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Implement color management on bdw/skl/bxt/kbl

2016-02-19 Thread kbuild test robot 
Hi Lionel,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160219]
[cannot apply to v4.5-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/drm-i915-Extract-out-gamma-table-and-CSC-to-their-own-file/20160219-234218
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s4-02201044 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

Note: the 
linux-review/Lionel-Landwerlin/drm-i915-Extract-out-gamma-table-and-CSC-to-their-own-file/20160219-234218
 HEAD 7fd974c72ceeb16885211256213d60c48bf2bdb7 builds fine.
  It only hurts bisectibility.

All error/warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/async.h:15,
from drivers/gpu/drm/i915/intel_drv.h:28,
from drivers/gpu/drm/i915/intel_color.c:25:
   drivers/gpu/drm/i915/intel_color.c: In function 'i9xx_load_luts':
>> drivers/gpu/drm/i915/intel_color.c:238:7: error: 'intel_crtc' undeclared 
>> (first use in this function)
  if (intel_crtc->config->has_dsi_encoder)
  ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
 if (__builtin_constant_p((cond)) ? !!(cond) :   \
   ^
>> drivers/gpu/drm/i915/intel_color.c:238:3: note: in expansion of macro 'if'
  if (intel_crtc->config->has_dsi_encoder)
  ^
   drivers/gpu/drm/i915/intel_color.c:238:7: note: each undeclared identifier 
is reported only once for each function it appears in
  if (intel_crtc->config->has_dsi_encoder)
  ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
 if (__builtin_constant_p((cond)) ? !!(cond) :   \
   ^
>> drivers/gpu/drm/i915/intel_color.c:238:3: note: in expansion of macro 'if'
  if (intel_crtc->config->has_dsi_encoder)
  ^

vim +/intel_crtc +238 drivers/gpu/drm/i915/intel_color.c

372867d6 Lionel Landwerlin 2016-02-19  232  struct drm_crtc_state *state = 
crtc->state;
5671ede9 Lionel Landwerlin 2016-02-19  233  struct drm_i915_private 
*dev_priv = dev->dev_private;
372867d6 Lionel Landwerlin 2016-02-19  234  enum pipe pipe = 
to_intel_crtc(crtc)->pipe;
5671ede9 Lionel Landwerlin 2016-02-19  235  int i;
5671ede9 Lionel Landwerlin 2016-02-19  236  
5671ede9 Lionel Landwerlin 2016-02-19  237  if (HAS_GMCH_DISPLAY(dev)) {
5671ede9 Lionel Landwerlin 2016-02-19 @238  if 
(intel_crtc->config->has_dsi_encoder)
5671ede9 Lionel Landwerlin 2016-02-19  239  
assert_dsi_pll_enabled(dev_priv);
5671ede9 Lionel Landwerlin 2016-02-19  240  else
5671ede9 Lionel Landwerlin 2016-02-19  241  
assert_pll_enabled(dev_priv, pipe);

:: The code at line 238 was first introduced by commit
:: 5671ede91a0c5e17d1d5f9c0e8c31f6357379a20 drm/i915: Extract out gamma 
table and CSC to their own file

:: TO: Lionel Landwerlin 
:: CC: 0day robot 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx