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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
