Hi Peter,
On Fri, 16 Jun 2017 11:12:25 +0200
Peter Rosin <peda@xxxxxxxxxx> wrote:
> All layers of chips support this, the only variable is the base address
> of the lookup table in the register map.
>
> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 48 +++++++++++++++++++++++++
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 13 +++++++
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 16 +++++++++
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +++
> 4 files changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 5348985..75871b5 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
> struct atmel_hlcdc_dc *dc;
> struct drm_pending_vblank_event *event;
> int id;
> + u32 clut[ATMEL_HLCDC_CLUT_SIZE];
Do we really need to duplicate this table here? I mean, the gamma_lut
table should always be available in the crtc_state, so do you have a
good reason a copy here?
> };
>
> static inline struct atmel_hlcdc_crtc *
> @@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
> cfg);
> }
>
> +static void
> +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
> +{
> + struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> + struct atmel_hlcdc_dc *dc = crtc->dc;
> + int layer;
> + int idx;
> +
> + for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
> + if (!dc->layers[layer])
> + continue;
> + for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
> + atmel_hlcdc_layer_write_clut(dc->layers[layer],
> + idx, crtc->clut[idx]);
> + }
> +}
> +
> +static void
> +atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
> +{
> + struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> + struct drm_crtc_state *state = c->state;
> + struct drm_color_lut *lut;
> + int idx;
> +
> + if (!state->gamma_lut)
> + return;
> +
> + lut = (struct drm_color_lut *)state->gamma_lut->data;
> +
> + for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
> + crtc->clut[idx] =
> + ((lut[idx].red << 8) & 0xff0000) |
> + (lut[idx].green & 0xff00) |
> + (lut[idx].blue >> 8);
> + }
> +
> + atmel_hlcdc_crtc_load_lut(c);
> +}
> +
> static enum drm_mode_status
> atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
> const struct drm_display_mode *mode)
> @@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_s)
> {
> /* TODO: write common plane control register if available */
> +
> + if (crtc->state->color_mgmt_changed)
> + atmel_hlcdc_crtc_flush_lut(crtc);
Hm, it's probably too late to do it here. Planes have already been
enabled and the engine may have started to fetch data and do the
composition. You could do that in ->update_plane() [1], and make it a
per-plane thing.
I'm not sure, but I think you can get the new crtc_state from
plane->crtc->state in this context (state have already been swapped,
and new state is being applied, which means relevant locks are held).
> }
>
> static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
> @@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
> .atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
> .enable_vblank = atmel_hlcdc_crtc_enable_vblank,
> .disable_vblank = atmel_hlcdc_crtc_disable_vblank,
> + .set_property = drm_atomic_helper_crtc_set_property,
Well, this change is independent from gamma LUT support. Should
probably be done in a separate patch.
Also, you should probably have:
.gamma_set = drm_atomic_helper_legacy_gamma_set,
> };
The rest looks good.
Thanks,
Boris
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
