Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191008230038.24037-1-ezequiel@collabora.com>
 <20191008230038.24037-3-ezequiel@collabora.com> <20191009180136.GE85762@art_vandelay>
 <CAAEAJfDP0PsGAoRfGyDyWj7DxgP6nwwwA1_gwLQuVy-fRDa-UA@mail.gmail.com> <20191010160059.GJ85762@art_vandelay>
In-Reply-To: <20191010160059.GJ85762@art_vandelay>
From:   Ilia Mirkin <imirkin@alum.mit.edu>
Date:   Thu, 10 Oct 2019 12:23:05 -0400
Message-ID: <CAKb7UvhWWYcpmyMZgerdJiG=sZjQUBVkeEwev+PdYzBW6+xsbQ@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT
To:     Sean Paul <sean@poorly.run>
Cc:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Ezequiel Garcia <ezequiel@collabora.com>,
        Mark Rutland <mark.rutland@arm.com>,
        devicetree <devicetree@vger.kernel.org>,
        Jacopo Mondi <jacopo@jmondi.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Douglas Anderson <dianders@chromium.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Boris Brezillon <boris.brezillon@collabora.com>,
        Sean Paul <seanpaul@chromium.org>,
        Rob Herring <robh+dt@kernel.org>, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 10, 2019 at 12:01 PM Sean Paul <sean@poorly.run> wrote:
> > > > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > > > +                              struct drm_crtc_state *crtc_state)
> > > > +{
> > > > +     struct vop *vop = to_vop(crtc);
> > > > +
> > > > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > > > +         crtc_state->gamma_lut) {
> > > > +             unsigned int len;
> > > > +
> > > > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > > > +             if (len != crtc->gamma_size) {
> > > > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > > > +                                   len, crtc->gamma_size);
> > > > +                     return -EINVAL;
> > > > +             }
> > >
> > > Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> > > this function.
> > >
> >
> > But that only applies to the legacy path. Isn't this needed to ensure
> > a gamma blob
> > has the right size?
>
> Yeah, good point, we check the element size in the atomic path, but not the max
> size. I haven't looked at enough color lut stuff to have an opinion whether this
> check would be useful in a helper function or not, something to consider, I
> suppose.

Some implementations support multiple sizes (e.g. 256 and 1024) but
not anything in between. It would be difficult to expose this
generically, I would imagine. The 256 size is kind of special, since
basically all legacy usage assumes that 256 is the one true quantity
of LUT entries...

  -ilia