linux-kernel - Re: [PATCH 2/3] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <33068f355edfaabb1c60d5e16a219a058a489531.camel@collabora.com>
Date:   Fri, 21 Jun 2019 12:52:51 -0300
From:   Ezequiel Garcia <ezequiel@...labora.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        "open list:ARM/Rockchip SoC..." <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>,
        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, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] drm/rockchip: Add optional support for CRTC gamma
 LUT

On Thu, 2019-06-20 at 10:25 -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2019 at 2:43 PM Ezequiel Garcia <ezequiel@...labora.com> wrote:
> > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > +                              struct drm_crtc_state *old_state)
> > +{
> > +       int idle, ret, i;
> > +
> > +       spin_lock(&vop->reg_lock);
> > +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > +       vop_cfg_done(vop);
> > +       spin_unlock(&vop->reg_lock);
> > +
> > +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > +                          idle, !idle, 5, 30 * 1000);
> > +       if (ret)
> 
> Worth an error message?
> 

Sure.

> 
> > @@ -1205,6 +1294,7 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
> > 
> >  static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
> >         .mode_fixup = vop_crtc_mode_fixup,
> > +       .atomic_check = vop_crtc_atomic_check,
> 
> At first I was worried that there was a bug here since in the context
> of dw_hdmi (an encoder) adding ".atomic_check" caused ".mode_fixup" to
> stop getting called as per mode_fixup() in
> "drivers/gpu/drm/drm_atomic_helper.c".
> 
> ...but it seems like it's OK for CRTCs, so I think we're fine.
> 
> 
> > @@ -1323,6 +1413,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
> >         .disable_vblank = vop_crtc_disable_vblank,
> >         .set_crc_source = vop_crtc_set_crc_source,
> >         .verify_crc_source = vop_crtc_verify_crc_source,
> > +       .gamma_set = drm_atomic_helper_legacy_gamma_set,
> 
> Are there any issues in adding this ".gamma_set" property even though
> we may or may not actually have the ability to set the gamma
> (depending on whether or not the LUT register range was provided in
> the device tree)?  I am a DRM noob but
> drm_atomic_helper_legacy_gamma_set() is not a trivial little function
> and now we'll be running it in some cases where we don't actually have
> gamma.
> 
> I also notice that there's at least one bit of code that seems to
> check if ".gamma_set" is NULL.  ...and if it is, it'll return -ENOSYS
> right away.  Do we still properly return -ENOSYS on devices that don't
> have the register range?
> 
> It seems like the absolute safest would be to have two copies of this
> struct: one used for VOPs that have the range and one for VOPs that
> don't.
> 
> ...but possibly I'm just paranoid and as I've said I'm a clueless
> noob.  If someone says it's fine to always provide the .gamma_set
> property that's fine too.
> 

Provided we do the suggestion below (setting gamma_size and enabling
color management) when lut_regs is set, then I think we are fine.

Before this change:

* GAMMA_LUT property doesn't exist, and so can't be set.
* DRM_IOCTL_MODE_SETGAMMA (legacy) return ENOSYS.

After this change, on platforms that doesn't support this:

* GAMMA_LUT property doesn't exist, and so can't be set.
* DRM_IOCTL_MODE_SETGAMMA (legacy) return EINVAL.

The only difference is the ENOSYS/EINVAL errno, which I doubt
will regress anything.

I don't think this difference deserves assigning (the legacy)
.gamma_set hook conditionally, which would make the
implementation too ugly.

> 
> >  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> > @@ -1480,6 +1571,10 @@ static int vop_create_crtc(struct vop *vop)
> >                 goto err_cleanup_planes;
> > 
> >         drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
> > +       if (vop_data->lut_size) {
> > +               drm_mode_crtc_set_gamma_size(crtc, vop_data->lut_size);
> > +               drm_crtc_enable_color_mgmt(crtc, 0, false, vop_data->lut_size);
> 
> Should we only do the above calls if we successfully mapped the resources?
> 

Yes, totally. See above.

> 
> > @@ -1776,6 +1871,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
> >         if (IS_ERR(vop->regs))
> >                 return PTR_ERR(vop->regs);
> > 
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lut");
> 
> As per comments in the bindings, shouldn't use the name "lut" but
> should just pick resource #1.

Yes.

Thanks a lot for the review,
Ezequiel