From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Jacopo Mondi <jacopo@jmondi.org> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>, kieran.bingham+renesas@ideasonboard.com, geert@linux-m68k.org, horms@verge.net.au, uli@fpond.eu, airlied@linux.ie, daniel@ffwll.ch, 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, Ulrich Hecht <uli+renesas@fpond.eu> Subject: Re: [PATCH v3 13/14] drm: rcar-du: kms: Update CMM in atomic commit tail Date: Tue, 27 Aug 2019 19:38:30 +0300 [thread overview] Message-ID: <20190827163830.GC5054@pendragon.ideasonboard.com> (raw) In-Reply-To: <20190827144421.vbcoizfjxj5ashv2@uno.localdomain> Hi Jacopo, On Tue, Aug 27, 2019 at 04:44:21PM +0200, Jacopo Mondi wrote: > On Tue, Aug 27, 2019 at 03:19:27AM +0300, Laurent Pinchart wrote: > > On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote: > >> On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote: > >>> Update CMM settings at in the atomic commit tail helper method. > >>> > >>> The CMM is updated with new gamma values provided to the driver > >>> in the GAMMA_LUT blob property. > >>> > >>> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++ > >>> 1 file changed, 35 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> index 61ca1d3c379a..047fdb982a11 100644 > >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> @@ -22,6 +22,7 @@ > >>> #include <linux/of_platform.h> > >>> #include <linux/wait.h> > >>> > >>> +#include "rcar_cmm.h" > >>> #include "rcar_du_crtc.h" > >>> #include "rcar_du_drv.h" > >>> #include "rcar_du_encoder.h" > >>> @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, > >>> * Atomic Check and Update > >>> */ > >>> > >>> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc, > >>> + struct drm_crtc_state *old_state) > >>> +{ > >>> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > >>> + struct rcar_cmm_config cmm_config = {}; > >>> + > >>> + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed) > >>> + return; > >>> + > >>> + if (!crtc->state->gamma_lut) { > >>> + cmm_config.lut.enable = false; > >>> + rcar_cmm_setup(rcrtc->cmm, &cmm_config); > >>> + > >>> + return; > >>> + } > >>> + > >>> + cmm_config.lut.enable = true; > >>> + cmm_config.lut.table = (struct drm_color_lut *) > >>> + crtc->state->gamma_lut->data; > >>> + > >>> + /* Set LUT table size to 0 if entries should not be updated. */ > >>> + if (!old_state->gamma_lut || > >>> + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id) > >>> + cmm_config.lut.size = crtc->state->gamma_lut->length > >>> + / sizeof(cmm_config.lut.table[0]); > >> > >> It has just occurred to me that the hardware only support LUTs of > > Where did you find this strict requirement ? I have tried programming > less than 256 entries in the 1-D LUT table, and it seems to me things > are working fine (from a visual inspection of the output image, I > don't see much differences from when I program the full table, maybe > that's an indication something is bad?) Or maybe a previous write of the full 256 entries has initialised the LUT correctly ? There's no hardware register telling how many LUT entries the hardware should use, and the documentation makes it quite clear that the LUT contains 256 entries. It is indexed by the values of the 8-bit pixel components, so it has to be written fully. > >> exactly 256 entries. Should we remove cmm_config.lut.size (simplifying > >> the code in the CMM driver), and add a check to the CRTC .atomic_check() > >> handler to reject invalid LUTs ? Sorry for not having caught this > >> earlier. > > > > Just an additional comment, if we drop the size field, then the > > cmm_config.lut.table pointer should be set to NULL when the LUT contents > > don't need to be updated. > > > >>> + else > >>> + cmm_config.lut.size = 0; > >>> + > >>> + rcar_cmm_setup(rcrtc->cmm, &cmm_config); > >>> +} > >>> + > >>> static int rcar_du_atomic_check(struct drm_device *dev, > >>> struct drm_atomic_state *state) > >>> { > >>> @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) > >>> rcdu->dpad1_source = rcrtc->index; > >>> } > >>> > >>> + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i) > >>> + rcar_du_atomic_commit_update_cmm(crtc, crtc_state); > >>> + > >>> /* Apply the atomic update. */ > >>> drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>> drm_atomic_helper_commit_planes(dev, old_state, -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Jacopo Mondi <jacopo@jmondi.org> Cc: muroya@ksk.co.jp, uli@fpond.eu, horms@verge.net.au, VenkataRajesh.Kalakodima@in.bosch.com, airlied@linux.ie, koji.matsuoka.xm@renesas.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, kieran.bingham+renesas@ideasonboard.com, geert@linux-m68k.org, Jacopo Mondi <jacopo+renesas@jmondi.org>, Harsha.ManjulaMallikarjun@in.bosch.com, Ulrich Hecht <uli+renesas@fpond.eu> Subject: Re: [PATCH v3 13/14] drm: rcar-du: kms: Update CMM in atomic commit tail Date: Tue, 27 Aug 2019 19:38:30 +0300 [thread overview] Message-ID: <20190827163830.GC5054@pendragon.ideasonboard.com> (raw) In-Reply-To: <20190827144421.vbcoizfjxj5ashv2@uno.localdomain> Hi Jacopo, On Tue, Aug 27, 2019 at 04:44:21PM +0200, Jacopo Mondi wrote: > On Tue, Aug 27, 2019 at 03:19:27AM +0300, Laurent Pinchart wrote: > > On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote: > >> On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote: > >>> Update CMM settings at in the atomic commit tail helper method. > >>> > >>> The CMM is updated with new gamma values provided to the driver > >>> in the GAMMA_LUT blob property. > >>> > >>> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++ > >>> 1 file changed, 35 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> index 61ca1d3c379a..047fdb982a11 100644 > >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> @@ -22,6 +22,7 @@ > >>> #include <linux/of_platform.h> > >>> #include <linux/wait.h> > >>> > >>> +#include "rcar_cmm.h" > >>> #include "rcar_du_crtc.h" > >>> #include "rcar_du_drv.h" > >>> #include "rcar_du_encoder.h" > >>> @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, > >>> * Atomic Check and Update > >>> */ > >>> > >>> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc, > >>> + struct drm_crtc_state *old_state) > >>> +{ > >>> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > >>> + struct rcar_cmm_config cmm_config = {}; > >>> + > >>> + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed) > >>> + return; > >>> + > >>> + if (!crtc->state->gamma_lut) { > >>> + cmm_config.lut.enable = false; > >>> + rcar_cmm_setup(rcrtc->cmm, &cmm_config); > >>> + > >>> + return; > >>> + } > >>> + > >>> + cmm_config.lut.enable = true; > >>> + cmm_config.lut.table = (struct drm_color_lut *) > >>> + crtc->state->gamma_lut->data; > >>> + > >>> + /* Set LUT table size to 0 if entries should not be updated. */ > >>> + if (!old_state->gamma_lut || > >>> + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id) > >>> + cmm_config.lut.size = crtc->state->gamma_lut->length > >>> + / sizeof(cmm_config.lut.table[0]); > >> > >> It has just occurred to me that the hardware only support LUTs of > > Where did you find this strict requirement ? I have tried programming > less than 256 entries in the 1-D LUT table, and it seems to me things > are working fine (from a visual inspection of the output image, I > don't see much differences from when I program the full table, maybe > that's an indication something is bad?) Or maybe a previous write of the full 256 entries has initialised the LUT correctly ? There's no hardware register telling how many LUT entries the hardware should use, and the documentation makes it quite clear that the LUT contains 256 entries. It is indexed by the values of the 8-bit pixel components, so it has to be written fully. > >> exactly 256 entries. Should we remove cmm_config.lut.size (simplifying > >> the code in the CMM driver), and add a check to the CRTC .atomic_check() > >> handler to reject invalid LUTs ? Sorry for not having caught this > >> earlier. > > > > Just an additional comment, if we drop the size field, then the > > cmm_config.lut.table pointer should be set to NULL when the LUT contents > > don't need to be updated. > > > >>> + else > >>> + cmm_config.lut.size = 0; > >>> + > >>> + rcar_cmm_setup(rcrtc->cmm, &cmm_config); > >>> +} > >>> + > >>> static int rcar_du_atomic_check(struct drm_device *dev, > >>> struct drm_atomic_state *state) > >>> { > >>> @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) > >>> rcdu->dpad1_source = rcrtc->index; > >>> } > >>> > >>> + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i) > >>> + rcar_du_atomic_commit_update_cmm(crtc, crtc_state); > >>> + > >>> /* Apply the atomic update. */ > >>> drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>> drm_atomic_helper_commit_planes(dev, old_state, -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-08-27 16:38 UTC|newest] Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-25 13:51 [PATCH v3 00/14] drm: rcar-du: Add Color Management Module (CMM) Jacopo Mondi 2019-08-25 13:51 ` [PATCH v3 01/14] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation Jacopo Mondi 2019-08-26 7:34 ` Geert Uytterhoeven 2019-08-26 7:34 ` Geert Uytterhoeven 2019-08-26 7:59 ` Jacopo Mondi 2019-08-26 7:59 ` Jacopo Mondi 2019-08-26 8:38 ` Geert Uytterhoeven 2019-08-26 8:38 ` Geert Uytterhoeven 2019-08-26 10:15 ` Laurent Pinchart 2019-08-26 10:15 ` Laurent Pinchart 2019-08-30 18:01 ` Jacopo Mondi 2019-08-30 18:01 ` Jacopo Mondi 2019-09-05 11:50 ` Laurent Pinchart 2019-09-05 11:50 ` Laurent Pinchart 2019-09-05 12:05 ` Geert Uytterhoeven 2019-09-05 12:05 ` Geert Uytterhoeven 2019-09-05 12:20 ` Laurent Pinchart 2019-09-05 12:20 ` Laurent Pinchart 2019-09-05 13:28 ` Jacopo Mondi 2019-09-05 13:28 ` Jacopo Mondi 2019-08-25 13:51 ` [PATCH v3 02/14] dt-bindings: display, renesas,du: Document cmms property Jacopo Mondi 2019-08-27 20:29 ` Rob Herring 2019-08-28 7:32 ` Geert Uytterhoeven 2019-08-28 7:32 ` [PATCH v3 02/14] dt-bindings: display, renesas, du: " Geert Uytterhoeven 2019-08-28 8:28 ` [PATCH v3 02/14] dt-bindings: display, renesas,du: " Laurent Pinchart 2019-08-28 8:28 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 03/14] arm64: dts: renesas: r8a7796: Add CMM units Jacopo Mondi 2019-08-26 7:28 ` Geert Uytterhoeven 2019-08-26 8:00 ` Jacopo Mondi 2019-08-26 22:43 ` Laurent Pinchart 2019-08-27 9:55 ` Jacopo Mondi 2019-08-27 10:12 ` Geert Uytterhoeven 2019-08-25 13:51 ` [PATCH v3 04/14] arm64: dts: renesas: r8a7795: " Jacopo Mondi 2019-08-26 22:45 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 05/14] arm64: dts: renesas: r8a77965: " Jacopo Mondi 2019-08-26 22:45 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 06/14] arm64: dts: renesas: r8a77990: " Jacopo Mondi 2019-08-26 22:47 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 07/14] arm64: dts: renesas: r8a77995: " Jacopo Mondi 2019-08-26 22:47 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 08/14] drm: rcar-du: Add support for CMM Jacopo Mondi 2019-08-26 7:31 ` Geert Uytterhoeven 2019-08-26 8:02 ` Jacopo Mondi 2019-08-27 0:24 ` Laurent Pinchart 2019-08-27 0:24 ` Laurent Pinchart 2019-08-27 14:56 ` Jacopo Mondi 2019-08-27 15:48 ` Jacopo Mondi 2019-08-27 16:34 ` Laurent Pinchart 2019-08-27 16:34 ` Laurent Pinchart 2019-09-05 9:57 ` Jacopo Mondi 2019-09-05 11:17 ` Laurent Pinchart 2019-09-05 13:14 ` Jacopo Mondi 2019-09-05 13:39 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 09/14] drm: rcar-du: Claim CMM support for Gen3 SoCs Jacopo Mondi 2019-08-25 13:51 ` [PATCH v3 10/14] drm: rcar-du: kms: Collect CMM instances Jacopo Mondi 2019-08-26 23:51 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 11/14] drm: rcar-du: crtc: Enable and disable CMMs Jacopo Mondi 2019-08-25 13:51 ` [PATCH v3 12/14] drm: rcar-du: crtc: Register GAMMA_LUT properties Jacopo Mondi 2019-08-25 13:51 ` [PATCH v3 13/14] drm: rcar-du: kms: Update CMM in atomic commit tail Jacopo Mondi 2019-08-27 0:00 ` Laurent Pinchart 2019-08-27 0:19 ` Laurent Pinchart 2019-08-27 14:44 ` Jacopo Mondi 2019-08-27 16:38 ` Laurent Pinchart [this message] 2019-08-27 16:38 ` Laurent Pinchart 2019-08-25 13:51 ` [PATCH v3 14/14] drm: rcar-du: Force CMM enablement when resuming Jacopo Mondi 2019-08-27 0:05 ` Laurent Pinchart 2019-09-05 10:58 ` Jacopo Mondi 2019-09-05 10:58 ` Jacopo Mondi 2019-09-05 11:25 ` Laurent Pinchart
Reply instructions: You may reply publicly 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=20190827163830.GC5054@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=Harsha.ManjulaMallikarjun@in.bosch.com \ --cc=VenkataRajesh.Kalakodima@in.bosch.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=geert@linux-m68k.org \ --cc=horms@verge.net.au \ --cc=jacopo+renesas@jmondi.org \ --cc=jacopo@jmondi.org \ --cc=kieran.bingham+renesas@ideasonboard.com \ --cc=koji.matsuoka.xm@renesas.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=muroya@ksk.co.jp \ --cc=uli+renesas@fpond.eu \ --cc=uli@fpond.eu \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.