Linux-Renesas-SoC Archive on lore.kernel.org
 help / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	kieran.bingham+renesas@ideasonboard.com, airlied@linux.ie,
	koji.matsuoka.xm@renesas.com, muroya@ksk.co.jp,
	VenkataRajesh.Kalakodima@in.bosch.com,
	Harsha.ManjulaMallikarjun@in.bosch.com,
	linux-renesas-soc@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties
Date: Fri, 14 Jun 2019 16:47:38 +0200
Message-ID: <20190614144738.GY23020@phenom.ffwll.local> (raw)
In-Reply-To: <20190614092745.wznk3iv5dgehmjsb@uno.localdomain>

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

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 14:22 [PATCH 00/20] drm: rcar-du: Add Color Management Module (CMM) Jacopo Mondi
2019-06-06 14:22 ` [PATCH 01/20] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation Jacopo Mondi
2019-06-06 16:52   ` Laurent Pinchart
2019-06-06 18:06   ` Geert Uytterhoeven
2019-06-06 20:19   ` Sergei Shtylyov
2019-06-06 14:22 ` [PATCH 02/20] dt-bindings: display, renesas,du: Document cmms property Jacopo Mondi
2019-06-06 16:52   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 03/20] dt-bindings: display, renesas,du: Update 'vsps' in example Jacopo Mondi
2019-06-06 16:53   ` Laurent Pinchart
2019-06-06 20:00     ` Geert Uytterhoeven
2019-06-07 11:36       ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 04/20] clk: renesas: r8a7796: Add CMM clocks Jacopo Mondi
2019-06-06 14:22 ` [PATCH 05/20] clk: renesas: r8a7795: " Jacopo Mondi
2019-06-06 16:58   ` Laurent Pinchart
2019-06-06 18:18     ` Jacopo Mondi
2019-06-12  7:41   ` Geert Uytterhoeven
2019-06-06 14:22 ` [PATCH 06/20] clk: renesas: r8a77965: " Jacopo Mondi
2019-06-06 16:59   ` Laurent Pinchart
2019-06-12  7:43   ` Geert Uytterhoeven
2019-06-06 14:22 ` [PATCH 07/20] clk: renesas: r8a77990: " Jacopo Mondi
2019-06-06 17:00   ` Laurent Pinchart
2019-06-12  7:43   ` Geert Uytterhoeven
2019-06-06 14:22 ` [PATCH 08/20] clk: renesas: r8a77995: " Jacopo Mondi
2019-06-06 17:00   ` Laurent Pinchart
2019-06-12  7:44   ` Geert Uytterhoeven
2019-06-06 14:22 ` [PATCH 09/20] arm64: dts: renesas: r8a7796: Add CMM units Jacopo Mondi
2019-06-06 17:04   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 10/20] arm64: dts: renesas: r8a7795: " Jacopo Mondi
2019-06-06 17:06   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 11/20] arm64: dts: renesas: r8a77965: " Jacopo Mondi
2019-06-06 17:06   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 12/20] arm64: dts: renesas: r8a77990: " Jacopo Mondi
2019-06-06 17:07   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 13/20] arm64: dts: renesas: r8a77995: " Jacopo Mondi
2019-06-06 17:08   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 14/20] drm: rcar-du: Add support for CMM Jacopo Mondi
2019-06-07 11:35   ` Laurent Pinchart
2019-06-07 11:44     ` Laurent Pinchart
2019-06-07 12:18     ` Laurent Pinchart
2019-06-14  8:54     ` Jacopo Mondi
2019-06-06 14:22 ` [PATCH 15/20] drm: rcar-du: Claim CMM support for Gen3 SoCs Jacopo Mondi
2019-06-07 11:38   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 16/20] drm: rcar-du: kms: Collect CMM instances Jacopo Mondi
2019-06-07 11:49   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 17/20] drm: rcar-du: crtc: Enable and disable CMMs Jacopo Mondi
2019-06-07 11:51   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 18/20] drm: rcar-du: group: Enable DU's CMM extension Jacopo Mondi
2019-06-07 11:55   ` Laurent Pinchart
2019-06-06 14:22 ` [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties Jacopo Mondi
2019-06-07 12:03   ` Laurent Pinchart
2019-06-14  8:15     ` Jacopo Mondi
2019-06-14  8:42       ` Daniel Vetter
2019-06-14  9:27         ` Jacopo Mondi
2019-06-14 14:47           ` Daniel Vetter [this message]
2019-06-06 14:22 ` [PATCH 20/20] drm: rcar-du: kms: Update CMM in atomic commit tail Jacopo Mondi
2019-06-07 12:06   ` Laurent Pinchart
2019-06-14  8:19     ` Jacopo Mondi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190614144738.GY23020@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=Harsha.ManjulaMallikarjun@in.bosch.com \
    --cc=VenkataRajesh.Kalakodima@in.bosch.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jacopo+renesas@jmondi.org \
    --cc=jacopo@jmondi.org \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=koji.matsuoka.xm@renesas.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=muroya@ksk.co.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox