| Message ID | 1499074930-10187-1-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-07-03 13:31, Boris Brezillon wrote: > On Mon, 3 Jul 2017 11:42:10 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> At init and if the gamma_lut property is ever removed, the clut >> registers must be programmed with a default gamma ramp instead of >> being left in some unknown state. >> >> Fixes: 364a7bf574eb ("drm: atmel-hlcdc: add support for 8-bit color lookup table mode") >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> index b5bd9b0..0ccd93c 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> @@ -429,6 +429,14 @@ static void atmel_hlcdc_plane_update_format(struct atmel_hlcdc_plane *plane, >> ATMEL_HLCDC_LAYER_FORMAT_CFG, cfg); >> } >> >> +static void atmel_hlcdc_default_gamma_ramp(struct atmel_hlcdc_layer *layer) >> +{ >> + int idx; >> + >> + for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) >> + atmel_hlcdc_layer_write_clut(layer, idx, idx * 0x10101); >> +} >> + >> static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) >> { >> struct drm_crtc *crtc = plane->base.crtc; >> @@ -438,9 +446,14 @@ static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) >> if (!crtc || !crtc->state) >> return; >> >> - if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut) >> + if (!crtc->state->color_mgmt_changed) >> return; >> >> + if (!crtc->state->gamma_lut) { >> + atmel_hlcdc_default_gamma_ramp(&plane->layer); > > Hm, I'd prefer to have state->gamma_lut properly initialized in > atmel_hlcdc_crtc_reset(), this way you don't have to do that in the > update path. The gamma_lut property can be removed, so you have to handle it here anyway. No? Cheers, peda >> + return; >> + } >> + >> lut = (struct drm_color_lut *)crtc->state->gamma_lut->data; >> >> for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++, lut++) { >> @@ -918,6 +931,8 @@ static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane, >> 0x40040890); >> } >> >> + atmel_hlcdc_default_gamma_ramp(&plane->layer); >> + >> return 0; >> } >> >
On 2017-07-03 14:02, Boris Brezillon wrote: > On Mon, 3 Jul 2017 13:53:28 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> On 2017-07-03 13:31, Boris Brezillon wrote: >>> On Mon, 3 Jul 2017 11:42:10 +0200 >>> Peter Rosin <peda@axentia.se> wrote: >>> >>>> At init and if the gamma_lut property is ever removed, the clut >>>> registers must be programmed with a default gamma ramp instead of >>>> being left in some unknown state. >>>> >>>> Fixes: 364a7bf574eb ("drm: atmel-hlcdc: add support for 8-bit color lookup table mode") >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 17 ++++++++++++++++- >>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>>> index b5bd9b0..0ccd93c 100644 >>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>>> @@ -429,6 +429,14 @@ static void atmel_hlcdc_plane_update_format(struct atmel_hlcdc_plane *plane, >>>> ATMEL_HLCDC_LAYER_FORMAT_CFG, cfg); >>>> } >>>> >>>> +static void atmel_hlcdc_default_gamma_ramp(struct atmel_hlcdc_layer *layer) >>>> +{ >>>> + int idx; >>>> + >>>> + for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) >>>> + atmel_hlcdc_layer_write_clut(layer, idx, idx * 0x10101); >>>> +} >>>> + >>>> static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) >>>> { >>>> struct drm_crtc *crtc = plane->base.crtc; >>>> @@ -438,9 +446,14 @@ static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) >>>> if (!crtc || !crtc->state) >>>> return; >>>> >>>> - if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut) >>>> + if (!crtc->state->color_mgmt_changed) >>>> return; >>>> >>>> + if (!crtc->state->gamma_lut) { >>>> + atmel_hlcdc_default_gamma_ramp(&plane->layer); >>> >>> Hm, I'd prefer to have state->gamma_lut properly initialized in >>> atmel_hlcdc_crtc_reset(), this way you don't have to do that in the >>> update path. >> >> The gamma_lut property can be removed, so you have to handle it here anyway. No? > > Hm, what do you mean by removed? AFAICT, a property, once attached to > a DRM object, exists until the DRM object is destroyed. The data > attached to this property (here, the gamma_lut array) can be NULL, but > once you have allocated the data container, it will be duplicated (and > possibly updated) every time an atomic operation is triggered. By remove I meant someone somehow triggering a call like this: drm_atomic_crtc_set_property(crtc, ...->gamma_lut_property, 0); By the looks of it, that is not happening? But I'm not sure of that, and even if it's not happening today, that may change... > By initializing this field in crtc->reset(), you enforce the > default/reset state, which IIUC, is what you want here. Agreed, if it is not possible to remove/clear out the gamma_lut property, then the only thing needed is to initialize the hw clut registers to the linear ramp somewhere early. But is it indeed a fact that there is no way to clear out the gamma_lut prop? Cheers, peda
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index b5bd9b0..0ccd93c 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -429,6 +429,14 @@ static void atmel_hlcdc_plane_update_format(struct atmel_hlcdc_plane *plane, ATMEL_HLCDC_LAYER_FORMAT_CFG, cfg); } +static void atmel_hlcdc_default_gamma_ramp(struct atmel_hlcdc_layer *layer) +{ + int idx; + + for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) + atmel_hlcdc_layer_write_clut(layer, idx, idx * 0x10101); +} + static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) { struct drm_crtc *crtc = plane->base.crtc; @@ -438,9 +446,14 @@ static void atmel_hlcdc_plane_update_clut(struct atmel_hlcdc_plane *plane) if (!crtc || !crtc->state) return; - if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut) + if (!crtc->state->color_mgmt_changed) return; + if (!crtc->state->gamma_lut) { + atmel_hlcdc_default_gamma_ramp(&plane->layer); + return; + } + lut = (struct drm_color_lut *)crtc->state->gamma_lut->data; for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++, lut++) { @@ -918,6 +931,8 @@ static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane, 0x40040890); } + atmel_hlcdc_default_gamma_ramp(&plane->layer); + return 0; }
At init and if the gamma_lut property is ever removed, the clut registers must be programmed with a default gamma ramp instead of being left in some unknown state. Fixes: 364a7bf574eb ("drm: atmel-hlcdc: add support for 8-bit color lookup table mode") Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)