| Message ID | 20170203131115.2058-2-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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ä <ville.syrjala@linux.intel.com> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.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. 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, > + INTEL_INFO(dev_priv)->color.gamma_lut_size, 0); > > intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT; > I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT); > -- > 2.9.3
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ä <ville.syrjala@linux.intel.com> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte > > 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, > > + 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, > > + INTEL_INFO(dev_priv)->color.gamma_lut_size, 0); > > > > intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT; > > I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT); > > -- > > 2.9.3 > >
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ä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte > > > 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. But it does require a cast to the specific type, so every user would have to roll their own thing. void* you don't have to cast at all. Of course you could by accident pass the wrong 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, > > > + 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, > > > + INTEL_INFO(dev_priv)->color.gamma_lut_size, 0); > > > > > > intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT; > > > I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT); > > > -- > > > 2.9.3 > > > >
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, + 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, + 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, + INTEL_INFO(dev_priv)->color.gamma_lut_size, 0); intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT; I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
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ä <ville.syrjala@linux.intel.com> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_color.c | 59 ++++++++++---------------------------- 1 file changed, 15 insertions(+), 44 deletions(-)