Re: [Intel-gfx] [PATCH 1/3] drm/i915: Merge BDW pipe gamma and degamma table code

[Ville Syrjälä]

On Fri, Feb 10, 2017 at 12:14:45PM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-02-09 at 16:47 +0200, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2017 at 03:11:13PM +0200, Ander Conselvan de Oliveira wrote:
> > > The only difference between the code loading the pipe gamma and degamma
> > > tables in BDW is that the gamma code also writes the registers that hold
> > > the maximum values. So we can use the gamma code for the degamma table,
> > > at the expense of writing the maximum value register twice, with
> > > potenttially wrong values in the first time.
> > > 
> > > v2: Pass PAL_PREC_SPLIT_MODE from the caller. (Ville)
> > > Cc: Ville Syrjälä 
> > > Signed-off-by: Ander Conselvan de Oliveira 
> > >  > > l.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_color.c | 59 
> > > ++-
> > > ---
> > >  1 file changed, 15 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > > b/drivers/gpu/drm/i915/intel_color.c
> > > index 0627eee..82e1809 100644
> > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > @@ -340,54 +340,20 @@ static void haswell_load_luts(struct drm_crtc_state
> > > *crtc_state)
> > >   hsw_enable_ips(intel_crtc);
> > >  }
> > >  
> > > -static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> > > +static void bdw_load_lut(struct drm_crtc_state *state, u32 offset,
> > 
> > Maybe just pass the intel_crtc instead of the state? I presume we don't
> > need anything else from the state really?
> > 
> > > +  struct drm_color_lut *lut, u32 lut_size,
> > > +  u32 flags)
> > >  {
> > >   struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> > >   enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > > - uint32_t i, lut_size = INTEL_INFO(dev_priv)-
> > > >color.degamma_lut_size;
> > > -
> > > - I915_WRITE(PREC_PAL_INDEX(pipe),
> > > -    PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> > > -
> > > - if (state->degamma_lut) {
> > > - struct drm_color_lut *lut =
> > > - (struct drm_color_lut *) state->degamma_lut->data;
> > > -
> > > - for (i = 0; i < lut_size; i++) {
> > > - uint32_t word =
> > > - drm_color_lut_extract(lut[i].red, 10) << 20 |
> > > - drm_color_lut_extract(lut[i].green, 10) << 10 |
> > > - drm_color_lut_extract(lut[i].blue, 10);
> > > -
> > > - I915_WRITE(PREC_PAL_DATA(pipe), word);
> > > - }
> > > - } else {
> > > - for (i = 0; i < lut_size; i++) {
> > > - uint32_t v = (i * ((1 << 10) - 1)) / (lut_size -
> > > 1);
> > > -
> > > - I915_WRITE(PREC_PAL_DATA(pipe),
> > > -    (v << 20) | (v << 10) | v);
> > > - }
> > > - }
> > > -}
> > > -
> > > -static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> > > -{
> > > - struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> > > - enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > > - uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > + uint32_t i;
> > >  
> > >   WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> > >  
> > >   I915_WRITE(PREC_PAL_INDEX(pipe),
> > > -    (offset ? PAL_PREC_SPLIT_MODE : 0) |
> > > -    PAL_PREC_AUTO_INCREMENT |
> > > -    offset);
> > > -
> > > - if (state->gamma_lut) {
> > > - struct drm_color_lut *lut =
> > > - (struct drm_color_lut *) state->gamma_lut->data;
> > > +    flags | PAL_PREC_AUTO_INCREMENT | offset);
> > >  
> > > + if (lut) {
> > >   for (i = 0; i < lut_size; i++) {
> > >   uint32_t word =
> > >   (drm_color_lut_extract(lut[i].red, 10) << 20) |
> > > @@ -430,9 +396,13 @@ static void broadwell_load_luts(struct drm_crtc_state
> > > *state)
> > >   return;
> > >   }
> > >  
> > > - bdw_load_degamma_lut(state);
> > > - bdw_load_gamma_lut(state,
> > > -    INTEL_INFO(dev_priv)->color.degamma_lut_size);
> > > + bdw_load_lut(state, 0, (struct drm_color_lut *) state->degamma_lut,
> > 
> > ->data ?
> > 
> > Such explicit casts are a good recipe for bugs I think, so would be nice
> > if someone could make this blob stuff more type safe.
> 
> Yikes! Totally missed that. Indeed the cast is a really bad idea.
> 
> > Dunno, maybe some kind of 'void *blob_get_data()' type of thing, or
> > maybe decouple the blob data from the blob once again so that we'd have
> > 'void *data' in there.
> 
> For this patch, how about a 
> 
> static struct drm_color_lut *
> blob_data_as_lut(struct drm_property_blob *blob)
> {
> if (!blob)
> return NULL;
> 
> return (struct drm_color_lut *) blob->data;
> }

I could live with that.

> 
> I think it is better than passing a void*.

Perhaps. 

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Merge BDW pipe gamma and degamma table code

[Ander Conselvan De Oliveira]

On Thu, 2017-02-09 at 16:47 +0200, Ville Syrjälä wrote:
> On Fri, Feb 03, 2017 at 03:11:13PM +0200, Ander Conselvan de Oliveira wrote:
> > The only difference between the code loading the pipe gamma and degamma
> > tables in BDW is that the gamma code also writes the registers that hold
> > the maximum values. So we can use the gamma code for the degamma table,
> > at the expense of writing the maximum value register twice, with
> > potenttially wrong values in the first time.
> > 
> > v2: Pass PAL_PREC_SPLIT_MODE from the caller. (Ville)
> > Cc: Ville Syrjälä 
> > Signed-off-by: Ander Conselvan de Oliveira  > l.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c | 59 ++-
> > ---
> >  1 file changed, 15 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index 0627eee..82e1809 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -340,54 +340,20 @@ static void haswell_load_luts(struct drm_crtc_state
> > *crtc_state)
> >     hsw_enable_ips(intel_crtc);
> >  }
> >  
> > -static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> > +static void bdw_load_lut(struct drm_crtc_state *state, u32 offset,
> 
> Maybe just pass the intel_crtc instead of the state? I presume we don't
> need anything else from the state really?
> 
> > +    struct drm_color_lut *lut, u32 lut_size,
> > +    u32 flags)
> >  {
> >     struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> >     enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > -   uint32_t i, lut_size = INTEL_INFO(dev_priv)-
> > >color.degamma_lut_size;
> > -
> > -   I915_WRITE(PREC_PAL_INDEX(pipe),
> > -      PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> > -
> > -   if (state->degamma_lut) {
> > -   struct drm_color_lut *lut =
> > -   (struct drm_color_lut *) state->degamma_lut->data;
> > -
> > -   for (i = 0; i < lut_size; i++) {
> > -   uint32_t word =
> > -   drm_color_lut_extract(lut[i].red, 10) << 20 |
> > -   drm_color_lut_extract(lut[i].green, 10) << 10 |
> > -   drm_color_lut_extract(lut[i].blue, 10);
> > -
> > -   I915_WRITE(PREC_PAL_DATA(pipe), word);
> > -   }
> > -   } else {
> > -   for (i = 0; i < lut_size; i++) {
> > -   uint32_t v = (i * ((1 << 10) - 1)) / (lut_size -
> > 1);
> > -
> > -   I915_WRITE(PREC_PAL_DATA(pipe),
> > -      (v << 20) | (v << 10) | v);
> > -   }
> > -   }
> > -}
> > -
> > -static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> > -{
> > -   struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> > -   enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > -   uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > +   uint32_t i;
> >  
> >     WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> >  
> >     I915_WRITE(PREC_PAL_INDEX(pipe),
> > -      (offset ? PAL_PREC_SPLIT_MODE : 0) |
> > -      PAL_PREC_AUTO_INCREMENT |
> > -      offset);
> > -
> > -   if (state->gamma_lut) {
> > -   struct drm_color_lut *lut =
> > -   (struct drm_color_lut *) state->gamma_lut->data;
> > +      flags | PAL_PREC_AUTO_INCREMENT | offset);
> >  
> > +   if (lut) {
> >     for (i = 0; i < lut_size; i++) {
> >     uint32_t word =
> >     (drm_color_lut_extract(lut[i].red, 10) << 20) |
> > @@ -430,9 +396,13 @@ static void broadwell_load_luts(struct drm_crtc_state
> > *state)
> >     return;
> >     }
> >  
> > -   bdw_load_degamma_lut(state);
> > -   bdw_load_gamma_lut(state,
> > -      INTEL_INFO(dev_priv)->color.degamma_lut_size);
> > +   bdw_load_lut(state, 0, (struct drm_color_lut *) state->degamma_lut,
> 
> ->data ?
> 
> Such explicit casts are a good recipe for bugs I think, so would be nice
> if someone could make this blob stuff more type safe.

Yikes! Totally missed that. Indeed the cast is a really bad idea.

> Dunno, maybe some kind of 'void *blob_get_data()' type of thing, or
> maybe decouple the blob data from the blob once again so that we'd have
> 'void *data' in there.

For this patch, how about a 

static struct drm_color_lut *
blob_data_as_lut(struct drm_property_blob *blob)
{
if (!blob)
return NULL;

return (struct drm_color_lut *) blob->data;
}

I think it is better than passing a void*.

Ander

> 
> > +    INTEL_INFO(dev_priv)->color.degamma_lut_size,
> > +    PAL_PREC_SPLIT_MODE);
> > +   bdw_load_lut(state, INTEL_INFO(dev_priv)->color.degamma_lut_size,
> > +    (struct drm_color_lut *) state->gamma_lut,
> > +      

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Merge BDW pipe gamma and degamma table code

[Ville Syrjälä]

On Fri, Feb 03, 2017 at 03:11:13PM +0200, Ander Conselvan de Oliveira wrote:
> The only difference between the code loading the pipe gamma and degamma
> tables in BDW is that the gamma code also writes the registers that hold
> the maximum values. So we can use the gamma code for the degamma table,
> at the expense of writing the maximum value register twice, with
> potenttially wrong values in the first time.
> 
> v2: Pass PAL_PREC_SPLIT_MODE from the caller. (Ville)
> Cc: Ville Syrjälä 
> Signed-off-by: Ander Conselvan de Oliveira 
> 
> ---
>  drivers/gpu/drm/i915/intel_color.c | 59 
> ++
>  1 file changed, 15 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c 
> b/drivers/gpu/drm/i915/intel_color.c
> index 0627eee..82e1809 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -340,54 +340,20 @@ static void haswell_load_luts(struct drm_crtc_state 
> *crtc_state)
>   hsw_enable_ips(intel_crtc);
>  }
>  
> -static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> +static void bdw_load_lut(struct drm_crtc_state *state, u32 offset,

Maybe just pass the intel_crtc instead of the state? I presume we don't
need anything else from the state really?

> +  struct drm_color_lut *lut, u32 lut_size,
> +  u32 flags)
>  {
>   struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>   enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> - uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> -
> - I915_WRITE(PREC_PAL_INDEX(pipe),
> -PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> -
> - if (state->degamma_lut) {
> - struct drm_color_lut *lut =
> - (struct drm_color_lut *) state->degamma_lut->data;
> -
> - for (i = 0; i < lut_size; i++) {
> - uint32_t word =
> - drm_color_lut_extract(lut[i].red, 10) << 20 |
> - drm_color_lut_extract(lut[i].green, 10) << 10 |
> - drm_color_lut_extract(lut[i].blue, 10);
> -
> - I915_WRITE(PREC_PAL_DATA(pipe), word);
> - }
> - } else {
> - for (i = 0; i < lut_size; i++) {
> - uint32_t v = (i * ((1 << 10) - 1)) / (lut_size - 1);
> -
> - I915_WRITE(PREC_PAL_DATA(pipe),
> -(v << 20) | (v << 10) | v);
> - }
> - }
> -}
> -
> -static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> -{
> - struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> - enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> - uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> + uint32_t i;
>  
>   WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>  
>   I915_WRITE(PREC_PAL_INDEX(pipe),
> -(offset ? PAL_PREC_SPLIT_MODE : 0) |
> -PAL_PREC_AUTO_INCREMENT |
> -offset);
> -
> - if (state->gamma_lut) {
> - struct drm_color_lut *lut =
> - (struct drm_color_lut *) state->gamma_lut->data;
> +flags | PAL_PREC_AUTO_INCREMENT | offset);
>  
> + if (lut) {
>   for (i = 0; i < lut_size; i++) {
>   uint32_t word =
>   (drm_color_lut_extract(lut[i].red, 10) << 20) |
> @@ -430,9 +396,13 @@ static void broadwell_load_luts(struct drm_crtc_state 
> *state)
>   return;
>   }
>  
> - bdw_load_degamma_lut(state);
> - bdw_load_gamma_lut(state,
> -INTEL_INFO(dev_priv)->color.degamma_lut_size);
> + bdw_load_lut(state, 0, (struct drm_color_lut *) state->degamma_lut,

->data ?

Such explicit casts are a good recipe for bugs I think, so would be nice
if someone could make this blob stuff more type safe.

Dunno, maybe some kind of 'void *blob_get_data()' type of thing, or
maybe decouple the blob data from the blob once again so that we'd have
'void *data' in there.

> +  INTEL_INFO(dev_priv)->color.degamma_lut_size,
> +  PAL_PREC_SPLIT_MODE);
> + bdw_load_lut(state, INTEL_INFO(dev_priv)->color.degamma_lut_size,
> +  (struct drm_color_lut *) state->gamma_lut,
> +  INTEL_INFO(dev_priv)->color.gamma_lut_size,
> +  PAL_PREC_SPLIT_MODE);
>  
>   intel_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
>   I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_SPLIT);
> @@ -489,7 +459,8 @@ static void glk_load_luts(struct drm_crtc_state *state)
>   }
>  
>   glk_load_degamma_lut(state);
> - bdw_load_gamma_lut(state, 0);
> + bdw_load_lut(state, 0, (struct drm_color_lut *) state->gamma_lut,
> +