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?).
>
> 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.
>
> >> };
> >>
> >> 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.
>
> >> }
> >>
> >> 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.
>
> > 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.
Regards,
Boris
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
