Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Boris Brezillon]

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