HI Laurent,

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:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > 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 <[email protected]>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > 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?)

Thanks
j
> > 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
>
> --
> Regards,
>
> Laurent Pinchart

Attachments:

(No filename) (3.96 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-27 16:40:10

by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH v3 13/14] drm: rcar-du: kms: Update CMM in atomic commit tail

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 <[email protected]>
> >>> Reviewed-by: Laurent Pinchart <[email protected]>
> >>> Signed-off-by: Jacopo Mondi <[email protected]>
> >>> ---
> >>> 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

2019-08-30 18:00:57

by jacopo mondi

[permalink] [raw]

Subject: Re: [PATCH v3 01/14] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Laurent,

On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 26, 2019 at 09:59:43AM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 26, 2019 at 09:34:41AM +0200, Geert Uytterhoeven wrote:
> > > On Sun, Aug 25, 2019 at 3:50 PM Jacopo Mondi <[email protected]> wrote:
> > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > Unit Color Management Module.
> > > >
> > > > CMM is the image enhancement module available on each R-Car DU video
> > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > >
> > > > Signed-off-by: Jacopo Mondi <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > @@ -0,0 +1,33 @@
> > > > +* Renesas R-Car Color Management Module (CMM)
> > > > +
> > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > +
> > > > +Required properties:
> > > > + - compatible: shall be one or more of the following:
> > > > + - "renesas,cmm-r8a7795": for R8A7795 (R-Car H3) compatible CMM.
> > > > + - "renesas,cmm-r8a7796": for R8A7796 (R-Car M3-W) compatible CMM.
> > > > + - "renesas,cmm-r8a77965": for R8A77965 (R-Car M3-N) compatible CMM.
> > > > + - "renesas,cmm-r8a77990": for R8A77990 (R-Car E3) compatible CMM.
> > > > + - "renesas,cmm-r8a77995": for R8A77995 (R-Car D3) compatible CMM.
> > >
> > > Please use "renesas,<socype->-cmm" instead of "renesas,cmm-<soctype>".
> >
> > I actually copied it from the r-car gpio bindings, and I liked
> > cmm-<soctype> better. If you prefer I can change it though.
> >
> > > > + - "renesas,rcar-gen3-cmm": for a generic R-Car Gen3 compatible CMM.
> > > > + - "renesas,rcar-gen2-cmm": for a generic R-Car Gen2 compatible CMM.
> > > > +
> > > > + When the generic compatible string is specified, the SoC-specific
> > > > + version corresponding to the platform should be listed first.
> > > > +
> > > > + - reg: the address base and length of the memory area where CMM control
> > > > + registers are mapped to.
> > > > +
> > > > + - clocks: phandle and clock-specifier pair to the CMM functional clock
> > > > + supplier.
> > >
> > > Thinking about yaml validation:
> > >
> > > power-domains?
> > > resets?
> >
> > They should indeed be documented.
>
> How about converting this binding to yaml alreay ? It should be fairly
> simple.

I'm trying to, and I'm having my portion of fun time at it.

The definition of the schema itself seems good, but I wonder, is this
the first renesas schema we have? Because it seems to me the schema
validator is having an hard time to digest the examplea 'clocks' and
'power-domains' properties, which have 1 phandle and 2 specifiers and 1
phandle and 1 specifier respectively for Rensas SoCs.

In other words, if in the example I have:

examples:
- |
cmm0: cmm@fea40000 {
compatible = "renesas,r8a7796-cmm";
reg = <0 0xfea40000 0 0x1000>;
clocks = <&cpg 711> <---- 1 phandle + 1 specifier
resets = <&cpg 711>;
power-domains = <&sysc>; <---- 1 phandle
};

The schema validation is good.

While if I use an actual example
- |
cmm0: cmm@fea40000 {
compatible = "renesas,r8a7796-cmm";
reg = <0 0xfea40000 0 0x1000>;
clocks = <&cpg CPG_MOD 711> <---- 1 phandle + 2 specifier
resets = <&cpg 711>;
power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle
}; + 1 specfier

The schema validation fails...
Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error
FATAL ERROR: Unable to parse input tree

Are clocks properties with > 2 entries and power-domains properties with
> 1 entries supported?

Because from what I read here:
https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.yaml
"The length of a clock specifier is defined by the value of a #clock-cells
property in the clock provider node."

And that's expected, but is the examples actually validated against the
clock provider pointed by the phandle? Because in that case, if we had a
yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.

Do we need a schema for cpg-mssr first, or am I doing something else
wrong?

Thanks
j

>
> > > > +Example:
> > > > +--------
> > > > +
> > > > + cmm0: cmm@fea40000 {
> > > > + compatible = "renesas,cmm-r8a7796";
> > > > + reg = <0 0xfea40000 0 0x1000>;
> > > > + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> > > > + clocks = <&cpg CPG_MOD 711>;
> > > > + resets = <&cpg 711>;
> > > > + };
>
> --
> Regards,
>
> Laurent Pinchart

Attachments:

(No filename) (4.88 kB)
signature.asc (849.00 B)
Download all attachments

2019-09-05 10:58:01

Hi Laurent, Geert,

On Thu, Sep 05, 2019 at 03:20:59PM +0300, Laurent Pinchart wrote:
> Hi Geert,
>
> On Thu, Sep 05, 2019 at 02:05:34PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Sep 5, 2019 at 1:50 PM Laurent Pinchart wrote:
> > > On Fri, Aug 30, 2019 at 08:01:09PM +0200, Jacopo Mondi wrote:
> > > > On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:
> > > > > How about converting this binding to yaml alreay ? It should be fairly
> > > > > simple.
> > > >
> > > > I'm trying to, and I'm having my portion of fun time at it.
> > > >
> > > > The definition of the schema itself seems good, but I wonder, is this
> > > > the first renesas schema we have? Because it seems to me the schema
> > > > validator is having an hard time to digest the examplea 'clocks' and
> > > > 'power-domains' properties, which have 1 phandle and 2 specifiers and 1
> > > > phandle and 1 specifier respectively for Rensas SoCs.
> > > >
> > > > In other words, if in the example I have:
> > > >
> > > > examples:
> > > > - |
> > > > cmm0: cmm@fea40000 {
> > > > compatible = "renesas,r8a7796-cmm";
> > > > reg = <0 0xfea40000 0 0x1000>;
> > > > clocks = <&cpg 711> <---- 1 phandle + 1 specifier
> > > > resets = <&cpg 711>;
> > > > power-domains = <&sysc>; <---- 1 phandle
> > > > };
> > > >
> > > > The schema validation is good.
> > > >
> > > > While if I use an actual example
> > > > - |
> > > > cmm0: cmm@fea40000 {
> > > > compatible = "renesas,r8a7796-cmm";
> > > > reg = <0 0xfea40000 0 0x1000>;
> > > > clocks = <&cpg CPG_MOD 711> <---- 1 phandle + 2 specifier
> > > > resets = <&cpg 711>;
> > > > power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle
> > > > }; + 1 specfier
> > > >
> > > > The schema validation fails...
> > > > Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error
> > > > FATAL ERROR: Unable to parse input tree
> > > >
> > > > Are clocks properties with > 2 entries and power-domains properties with
> > > > > 1 entries supported?
> > > >
> > > > Because from what I read here:
> > > > https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.yaml
> > > > "The length of a clock specifier is defined by the value of a #clock-cells
> > > > property in the clock provider node."
> > > >
> > > > And that's expected, but is the examples actually validated against the
> > > > clock provider pointed by the phandle? Because in that case, if we had a
> > > > yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.
> > > >
> > > > Do we need a schema for cpg-mssr first, or am I doing something else
> > > > wrong?
> > >
> > > I think you just need to #include the headers that define CPG_MOD and
> > > R8A7796_PD_ALWAYS_ON.
> >
> > If that helps, you might want to replace the symbols in the examples by their
> > actual values (1 resp. 32).
> >
> > And perhaps keep the symbols as comments?
> >
> > clocks = <&cpg 1 /* CPG_MOD */ 711>;
> > power-domains = <&sysc 32 /* R8A7796_PD_ALWAYS_ON */>;
>
> I think adding the required #include at the beginning of the example is
> a better solution.

I didn't realize that, but it actually makes sense, as the example is
extracted and actually compiled from the yaml schema.. brilliant!

With a simple:

--- a/Documentation/devicetree/bindings/display/renesas,cmm.yaml
+++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml
@@ -51,6 +51,9 @@ additionalProperties: false

examples:
- |
+ #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
+ #include <dt-bindings/power/r8a7796-sysc.h>

The example now compiles.

Thanks, I will submit the bindings in yaml format in next iteration.

>
> --
> Regards,
>
> Laurent Pinchart

Attachments:

(No filename) (3.93 kB)
signature.asc (849.00 B)
Download all attachments

2019-09-05 16:44:55

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v3 01/14] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Laurent,

On Thu, Sep 5, 2019 at 1:50 PM Laurent Pinchart
<[email protected]> wrote:
> On Fri, Aug 30, 2019 at 08:01:09PM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:
> > > How about converting this binding to yaml alreay ? It should be fairly
> > > simple.
> >
> > I'm trying to, and I'm having my portion of fun time at it.
> >
> > The definition of the schema itself seems good, but I wonder, is this
> > the first renesas schema we have? Because it seems to me the schema
> > validator is having an hard time to digest the examplea 'clocks' and
> > 'power-domains' properties, which have 1 phandle and 2 specifiers and 1
> > phandle and 1 specifier respectively for Rensas SoCs.
> >
> > In other words, if in the example I have:
> >
> > examples:
> > - |
> > cmm0: cmm@fea40000 {
> > compatible = "renesas,r8a7796-cmm";
> > reg = <0 0xfea40000 0 0x1000>;
> > clocks = <&cpg 711> <---- 1 phandle + 1 specifier
> > resets = <&cpg 711>;
> > power-domains = <&sysc>; <---- 1 phandle
> > };
> >
> > The schema validation is good.
> >
> > While if I use an actual example
> > - |
> > cmm0: cmm@fea40000 {
> > compatible = "renesas,r8a7796-cmm";
> > reg = <0 0xfea40000 0 0x1000>;
> > clocks = <&cpg CPG_MOD 711> <---- 1 phandle + 2 specifier
> > resets = <&cpg 711>;
> > power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle
> > }; + 1 specfier
> >
> > The schema validation fails...
> > Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error
> > FATAL ERROR: Unable to parse input tree
> >
> > Are clocks properties with > 2 entries and power-domains properties with
> > > 1 entries supported?
> >
> > Because from what I read here:
> > https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.yaml
> > "The length of a clock specifier is defined by the value of a #clock-cells
> > property in the clock provider node."
> >
> > And that's expected, but is the examples actually validated against the
> > clock provider pointed by the phandle? Because in that case, if we had a
> > yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.
> >
> > Do we need a schema for cpg-mssr first, or am I doing something else
> > wrong?
>
> I think you just need to #include the headers that define CPG_MOD and
> R8A7796_PD_ALWAYS_ON.

If that helps, you might want to replace the symbols in the examples by their
actual values (1 resp. 32).

And perhaps keep the symbols as comments?

clocks = <&cpg 1 /* CPG_MOD */ 711>;
power-domains = <&sysc 32 /* R8A7796_PD_ALWAYS_ON */>;

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds