/snip
> > > +static void vop_crtc_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
> > > +{
> > > + struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < crtc->gamma_size; i++) {
> > > + u32 word;
> > > +
> > > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
> > > + (drm_color_lut_extract(lut[i].green, 10) << 10) |
> > > + drm_color_lut_extract(lut[i].blue, 10);
> > > + writel(word, vop->lut_regs + i * 4);
> > > + }
> > > +}
> > > +
> > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > > + struct drm_crtc_state *old_crtc_state)
> > > +{
> > > + unsigned int idle;
> > > + int ret;
> > > +
> >
> > How about:
> >
> > if (!vop->lut_regs)
> > return;
> >
> > here and then you can remove that condition above the 2 callsites
> >
>
> Yes, sounds good.
>
> > > + /*
> > > + * In order to write the LUT to the internal memory,
> > > + * we need to first make sure the dsp_lut_en bit is cleared.
> > > + */
> > > + spin_lock(&vop->reg_lock);
> > > + VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > > + vop_cfg_done(vop);
> > > + spin_unlock(&vop->reg_lock);
> > > +
> > > + /*
> > > + * If the CRTC is not active, dsp_lut_en will not get cleared.
> > > + * Apparently we still need to do the above step to for
> > > + * gamma correction to be disabled.
> > > + */
> > > + if (!crtc->state->active)
> > > + return;
> > > +
>
> I have realized that the above might no longer be needed,
> given we are now using atomic_enable and atomic_begin.
>
> Not sure if the CRTC is supposed to clear its GAMMA
> when disabled.
>
Yep, good catch. Since we use commit_tail_rpm, atomic_begin won't be called in
the disable path.
> > > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > > + idle, !idle, 5, 30 * 1000);
> > > + if (ret) {
> > > + DRM_DEV_ERROR(vop->dev, "display LUT RAM enable timeout!\n");
> > > + return;
> > > + }
> > > +
> > > + if (crtc->state->gamma_lut &&
> > > + (!old_crtc_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > > + old_crtc_state->gamma_lut->base.id))) {
> >
> > Silly question, but isn't the second part of this check redundant since you need
> > color_mgmt_changed || active_changed to get into this function?
> >
> > So maybe invert the conditional here and exit early (to save a level of
> > indentation in the block below):
> >
>
> I took this from malidp_atomic_commit_update_gamma. I _believe_
> the rational for this is that color_mgmt_changed can be set by re-setting
> the gamma property, to the same property. But I admit I haven't
> tested it's the case.
>
> OTOH, it won't really affect much to re-write the table, if the user
> requested a change.
>
color_mgmt_changed is based on the output of drm_property_replace_blob() which
should return false if the blob is unchanged. So I don't think that case is
possible.
Taking this a step further, this check could even be damaging since something
in the atomic check path could set color_mgmt_changed in order to explicitly
trigger a lut write and we'd be skipping it with the id check.
> > if (!crtc->state->gamma_lut)
> > return;
> >
>
> In any case, inverting the condition makes sense.
>
> > spin_lock(&vop->reg_lock);
> >
> > vop_crtc_write_gamma_lut(vop, crtc);
> > VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > vop_cfg_done(vop);
> >
> > spin_unlock(&vop->reg_lock);
> >
> > > +
> > > + spin_lock(&vop->reg_lock);
> > > +
> > > + vop_crtc_write_gamma_lut(vop, crtc);
> > > + VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > > + vop_cfg_done(vop);
> > > +
> > > + spin_unlock(&vop->reg_lock);
> > > + }
> > > +}
/snip
> > > +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.
Sean
>
> Thanks,
> Ezequiel
--
Sean Paul, Software Engineer, Google / Chromium OS
