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

[Peter Rosin]

On 2017-06-16 18:15, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 16 Jun 2017 17:54:04 +0200
> Peter Rosin <peda@xxxxxxxxxx> wrote:
> 
>> 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?
> 
> Can't we re-use crtc->gamma_store? Honnestly, I don't know how the
> fbdev->DRM link should be done, so we'd better wait for DRM maintainers
> feedback here (Daniel, any opinion?).

Ahh, gamma_store. Makes perfect sense. Thanks for that pointer!

>>
>> Sure, I could have added it in patch 3/3 instead, but didn't when I
>> divided the work into patches...
> 
> No, my point is that IMO it shouldn't be needed at all.

Right, with gamma_store it's no longer needed.

>>
>>>>  };
>>>>  
>>>>  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?
> 
> Why would you copy the default ->update_plane() when we already have
> our own ->atomic_update_plane() implementation [1]? Just put it there
> (before the atmel_hlcdc_layer_update_commit() call) and we should be
> good.

Ahh, but you said ->update_plane() and I took that as .update_plane in
layer_plane_funcs, not ->atomic_update() in atmel_hlcdc_layer_plane_helper_funcs.

Makes sense now, and much neater too.

>>
>>>>  }
>>>>  
>>>>  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...
> 
> It's probably a good thing to have it set anyway.

Looking at the code, I think it's needed since I think that's how the
gamma_lut property is modified for the non-emulated case...

>>
>>> 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.
> 
> To be very clear, I'd like you to test it through DRM ioctls, not only
> through the fbdev emulation layer.

...so yeah, right, I couldn't agree more. Any pointers to code w/o a bunch
of complex library dependencies that I can test with?

Cheers,
peda