linux-kernel - Re: [PATCH v3 3/5] drm/rockchip: Add optional support for CRTC gamma LUT

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]

Date:   Tue, 08 Oct 2019 16:23:36 -0300
From:   Ezequiel Garcia <ezequiel@...labora.com>
To:     Sean Paul <sean@...rly.run>
Cc:     dri-devel@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org,
        Heiko Stübner <heiko@...ech.de>,
        Sandy Huang <hjc@...k-chips.com>, kernel@...labora.com,
        Sean Paul <seanpaul@...omium.org>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        Douglas Anderson <dianders@...omium.org>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Ilia Mirkin <imirkin@...m.mit.edu>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jacopo Mondi <jacopo+renesas@...ndi.org>
Subject: Re: [PATCH v3 3/5] drm/rockchip: Add optional support for CRTC
 gamma LUT

Hello Sean,

On Mon, 2019-10-07 at 14:54 -0400, Sean Paul wrote:
> On Mon, Sep 30, 2019 at 07:28:00PM -0300, Ezequiel Garcia wrote:
> > Add an optional CRTC gamma LUT support, and enable it on RK3288.
> > This is currently enabled via a separate address resource,
> > which needs to be specified in the devicetree.
> > 
> > The address resource is required because on some SoCs, such as
> > RK3288, the LUT address is after the MMU address, and the latter
> > is supported by a different driver. This prevents the DRM driver
> > from requesting an entire register space.
> > 
> > The current implementation works for RGB 10-bit tables, as that
> > is what seems to work on RK3288.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
> > Reviewed-by: Douglas Anderson <dianders@...omium.org>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> > ---
> > Changes from v2:
> > * None.
> > 
> > Changes from v1:
> > * drop explicit linear LUT after finding a proper
> >   way to disable gamma correction.
> > * avoid setting gamma is the CRTC is not active.
> > * s/int/unsigned int as suggested by Jacopo.
> > * only enable color management and set gamma size
> >   if gamma LUT is supported, suggested by Doug.
> > * drop the reg-names usage, and instead just use indexed reg
> >   specifiers, suggested by Doug.
> > 
> > Changes from RFC:
> > * Request (an optional) address resource for the LUT.
> > * Drop support for RK3399, which doesn't seem to work
> >   out of the box and needs more research.
> > * Support pass-thru setting when GAMMA_LUT is NULL.
> > * Add a check for the gamma size, as suggested by Ilia.
> > * Move gamma setting to atomic_commit_tail, as pointed
> >   out by Jacopo/Laurent, is the correct way.
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   3 +
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 114 ++++++++++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   7 ++
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
> >  4 files changed, 126 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index dba352ec0ee3..fd1d987698ab 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -17,6 +17,7 @@
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_fb.h"
> >  #include "rockchip_drm_gem.h"
> > +#include "rockchip_drm_vop.h"
> >  
> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >  	.destroy       = drm_gem_fb_destroy,
> > @@ -112,6 +113,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >  
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >  
> > +	rockchip_drm_vop_gamma_set(old_state);
> > +
> 
> Instead of duplicating the commit_tail helper, could you just implement
> .atomic_begin() and call this from there? I think the only hitch is if you
> need this to be completed before crtc->atomic_enable(), at which point you
> might need to call it from vop_crtc_atomic_enable() and then detect that in
> atomic_begin()
> 

I think moving this to .atomic_begin might be enough. Let me send a new
series and we can see how that goes.

Thanks for reviewing,
Ezequiel