Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Ilia Mirkin]

On Thu, Oct 10, 2019 at 12:01 PM Sean Paul <sean@xxxxxxxxxx> 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