Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 14, 2019 at 11:27:45AM +0200, Jacopo Mondi wrote:
> Hi Daniel,
> 
> On Fri, Jun 14, 2019 at 10:42:51AM +0200, Daniel Vetter wrote:
> > On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:
> > > Hi Laurent,
> > >    thanks for review
> > >
> > > On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> > > > > Enable the GAMMA_LUT KMS property using the framework helpers to
> > > > > register the proeprty and the associated gamma table size maximum size.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > index e6d3df37c827..c920fb5dba65 100644
> > > > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> > > > >  	    rcdu->cmms[swindex]) {
> > > > >  		rcrtc->cmm = rcdu->cmms[swindex];
> > > > >  		rgrp->cmms_mask |= BIT(hwindex % 2);
> > > > > +
> > > > > +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> > > > > +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> > > >
> > > > This change looks good, but you also need to add support for legacy API.
> > > > According to the function's documentation,
> > > >
> > > >  * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
> > > >  * legacy &drm_crtc_funcs.gamma_set callback.
> > > >
> > >
> > > Drivers 'shuld' or drivers 'shall' ?
> > > Isn't this required only to support the 'legacy APIs' ? Do we want that?
> >
> > You're calling drm_mode_crtc_set_gamma_size, which is only useful for the
> > legacy ioctls. should here = assuming your hw supports something that
> > legacy gamma ioctl can use. Feel free to patch up the docs.
> 
> Oh, I see. I should either leave the old API alone without calling
> drm_mode_crtc_set_gamma_size(), or set the .gamma_set callback to
> point to drm_atomic_helper_legacy_gamma_set(), which translates the
> old gamma table interface to a blob property and attach it to a crtc
> state which is then commited and applied through the atomic helpers.
> 
> So I would change the doc to prescribe that if the driver intends to
> support the legacy SETGAMMA/GETGAMMA IOCTLs it should declare the
> gamma table size with drm_mode_crtc_set_gamma_size() first, and set
> the .gamma_set crtc callback to drm_atomic_helper_legacy_gamma_set(),
> which translates the legacy interface to a GAMMA_LUT property blob
> and commit it.
> 
> If that works, I'll make a small patch to the documentation in v2.

Not quite what I meant: You should support the legacy gamma ioctl, if your
gamma ramp can be squared into support that. Which generally means yes.
We've been thinking about just wiring this all up by default, but there's
some drivers who use a 256 entry legacy gamma ramp (for backwards compat
with old X11 userspace), but advertise much bigger tables through the new
ioctl. So it's not quite as simple.

But except if you have some really strange hw there's just no good reason
not to support the old legacy ioctl. We also don't just support the new
atomic ioctl on new drivers, they all still support the older interfaces.
This is the same.

That's what I meant should be clarified if it's not yet clear in docs,
plus maybe a new todo entry in Documentation/gpu/todo.rst.
-Daniel

> 
> Thanks
>   j
> 
> 
> > -Daniel
> >
> > >
> > > Thanks
> > >    j
> > >
> > > > >  	}
> > > > >
> > > > >  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> > > > >
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch