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

[Peter Rosin]

On 2017-06-16 12:01, Boris Brezillon wrote:
> 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?

If I don't keep a copy in the driver, it doesn't work when there's no
gamma_lut. And there is no gamma_lut when I use fbdev emulation. Maybe
that's a bug somewhere else?

Sure, I could have added it in patch 3/3 instead, but didn't when I
divided the work into patches...

>>  };
>>  
>>  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).

Ok, I can move it there. My plan is to just copy the default .update_plane
function and insert 

	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
		...
	}

just before the drm_atomic_commit(state) call. Sounds ok?

>>  }
>>  
>>  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.

Ok, I think I fat-fingered some kernel cmdline at some point and fooled
myself into thinking I needed it for some reason...

> Also, you should probably have:
>  
> 	.gamma_set = drm_atomic_helper_legacy_gamma_set,

That doesn't no anything for me, but sure, I can add it.

Cheers,
peda

>>  };
> 
> The rest looks good.
> 
> Thanks,
> 
> Boris
>