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@xxxxxxxxxxxxx> 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
