Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail
Hi Jacopo,
On Thu, Aug 22, 2019 at 09:19:25PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 20, 2019 at 09:42:15PM +0300, Laurent Pinchart wrote:
> > On Sat, Jul 06, 2019 at 04:07:46PM +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.
> > >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > 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 b79cda2f5531..f9aece78ca5f 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > @@ -21,6 +21,7 @@
> > > #include
> > > #include
> > >
> > > +#include "rcar_cmm.h"
> > > #include "rcar_du_crtc.h"
> > > #include "rcar_du_drv.h"
> > > #include "rcar_du_encoder.h"
> > > @@ -287,6 +288,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, _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]);
> > > + else
> > > + cmm_config.lut.size = 0;
> > > +
> > > + rcar_cmm_setup(rcrtc->cmm, _config);
> > > +}
> > > +
> > > static int rcar_du_atomic_check(struct drm_device *dev,
> > > struct drm_atomic_state *state)
> > > {
> > > @@ -329,6 +361,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);
> > > +
> >
> > I think this looks good overall, but I wonder if we couldn't simplify
> > the CMM driver suspend/resume and LUT caching due to config while not
> > enabled by handling it on the DU side. I have a rework on the commit
> > tail handler in progress, I'll think how this could be done. For now I
> > think you can leave it as is.
>
> Does this mean I have your R-b tag ? :)
I'd like to review this in the context of v3 :-)
> > > /* Apply the atomic update. */
> > > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > > drm_atomic_helper_commit_planes(dev, old_state,
--
Regards,
Laurent Pinchart
Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail
Hi Laurent,
On Tue, Aug 20, 2019 at 09:42:15PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sat, Jul 06, 2019 at 04:07:46PM +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.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > 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 b79cda2f5531..f9aece78ca5f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> >
> > +#include "rcar_cmm.h"
> > #include "rcar_du_crtc.h"
> > #include "rcar_du_drv.h"
> > #include "rcar_du_encoder.h"
> > @@ -287,6 +288,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, _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]);
> > + else
> > + cmm_config.lut.size = 0;
> > +
> > + rcar_cmm_setup(rcrtc->cmm, _config);
> > +}
> > +
> > static int rcar_du_atomic_check(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > {
> > @@ -329,6 +361,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);
> > +
>
> I think this looks good overall, but I wonder if we couldn't simplify
> the CMM driver suspend/resume and LUT caching due to config while not
> enabled by handling it on the DU side. I have a rework on the commit
> tail handler in progress, I'll think how this could be done. For now I
> think you can leave it as is.
>
Does this mean I have your R-b tag ? :)
Thanks
j
> > /* Apply the atomic update. */
> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > drm_atomic_helper_commit_planes(dev, old_state,
>
> --
> Regards,
>
> Laurent Pinchart
signature.asc
Description: PGP signature
Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail
Hi Jacopo,
Thank you for the patch.
On Sat, Jul 06, 2019 at 04:07:46PM +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.
>
> Signed-off-by: Jacopo Mondi
> ---
> 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 b79cda2f5531..f9aece78ca5f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -21,6 +21,7 @@
> #include
> #include
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -287,6 +288,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, _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]);
> + else
> + cmm_config.lut.size = 0;
> +
> + rcar_cmm_setup(rcrtc->cmm, _config);
> +}
> +
> static int rcar_du_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> @@ -329,6 +361,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);
> +
I think this looks good overall, but I wonder if we couldn't simplify
the CMM driver suspend/resume and LUT caching due to config while not
enabled by handling it on the DU side. I have a rework on the commit
tail handler in progress, I'll think how this could be done. For now I
think you can leave it as is.
> /* 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
Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail
> On July 6, 2019 at 4:07 PM 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.
>
> Signed-off-by: Jacopo Mondi
> ---
> 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 b79cda2f5531..f9aece78ca5f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -21,6 +21,7 @@
> #include
> #include
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -287,6 +288,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, _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]);
> + else
> + cmm_config.lut.size = 0;
> +
> + rcar_cmm_setup(rcrtc->cmm, _config);
> +}
> +
> static int rcar_du_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> @@ -329,6 +361,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,
> --
> 2.21.0
>
Reviewed-by: Ulrich Hecht
CU
Uli
[PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail
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.
Signed-off-by: Jacopo Mondi
---
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 b79cda2f5531..f9aece78ca5f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -21,6 +21,7 @@
#include
#include
+#include "rcar_cmm.h"
#include "rcar_du_crtc.h"
#include "rcar_du_drv.h"
#include "rcar_du_encoder.h"
@@ -287,6 +288,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, _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]);
+ else
+ cmm_config.lut.size = 0;
+
+ rcar_cmm_setup(rcrtc->cmm, _config);
+}
+
static int rcar_du_atomic_check(struct drm_device *dev,
struct drm_atomic_state *state)
{
@@ -329,6 +361,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,
--
2.21.0
