linux-kernel - Re: [PATCH] drm: atmel-hlcdc: use a default gamma ramp if none is specified

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170703140235.53329a09@bbrezillon>
Date:   Mon, 3 Jul 2017 14:02:35 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Peter Rosin <peda@...ntia.se>
Cc:     linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH] drm: atmel-hlcdc: use a default gamma ramp if none is
 specified

On Mon, 3 Jul 2017 13:53:28 +0200
Peter Rosin <peda@...ntia.se> wrote:

> On 2017-07-03 13:31, Boris Brezillon wrote:
> > On Mon,  3 Jul 2017 11:42:10 +0200
> > Peter Rosin <peda@...ntia.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@...ntia.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 initializing this field in crtc->reset(), you enforce the
default/reset state, which IIUC, is what you want here.