Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667607 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup  table mode Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-16 12:10 +0200
    Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup  table mode Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-16 18:20 +0200
      Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup  table mode Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-17 07:40 +0200

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

Hi Peter,

On Fri, 16 Jun 2017 11:12:25 +0200
Peter Rosin <peda@axentia.se> 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@axentia.se>
> ---
>  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

[toc] | [next] | [standalone]

#1667866

Hi Peter,

On Fri, 16 Jun 2017 17:54:04 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2017-06-16 12:01, Boris Brezillon wrote:
> > Hi Peter,
> > 
> > On Fri, 16 Jun 2017 11:12:25 +0200
> > Peter Rosin <peda@axentia.se> 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@axentia.se>
> >> ---
> >>  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

[toc] | [prev] | [next] | [standalone]

#1668184

Le Sat, 17 Jun 2017 00:46:12 +0200,
Peter Rosin <peda@axentia.se> a écrit :

> >>>> 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.  
> 
> No, it doesn't make sense. There's no atmel_hlcdc_layer_update_commit call
> anywhere, and no such function. You seem to have some further changes that
> are not even in -next. Where am I getting those changes and why are they
> not upstream yet?

My bad, this part as been reworked in 4.12 and I was reading 4.11 code.
Indeed, atmel_hlcdc_layer_update_commit() no longer exists, but
atmel_hlcdc_plane_atomic_update() does.

Just add a function called atmel_hlcdc_plane_update_clut() in
atmel_hlcdc_plane.c and call it just after [1].

[1]http://elixir.free-electrons.com/linux/v4.12-rc5/source/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c#L770

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web