| Message ID | 20201221015730.28333-2-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Add cubic LUT support | expand |
Hi Laurent, On 21/12/2020 01:57, Laurent Pinchart wrote: > To prepare for CLU support, expend the CMM API exposed to the DU driver s/expend/extend/ ...? > to separate the LUT table pointer from the LUT update decision. This > will be required, as we will need to update the LUT and CLU > independently. > Aha, I see this has changed a little since I originally looked at this, but that's probably a good thing. > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_cmm.c | 60 ++++++++++++-------------- > drivers/gpu/drm/rcar-du/rcar_cmm.h | 19 +++++--- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++--- > 3 files changed, 55 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c > index 382d53f8a22e..ccc8c8b03bac 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c > @@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) > iowrite32(data, rcmm->base + reg); > } > > -/* > - * rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision > - * and write to the CMM registers > - * @rcmm: Pointer to the CMM device > - * @drm_lut: Pointer to the DRM LUT table > - */ > -static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, > - const struct drm_color_lut *drm_lut) > +static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm, > + const struct drm_color_lut *table) > { > - unsigned int i; > + bool enable = !!table; > Ahh good, handling both enable and disable in here makes more sense. I like it. > - for (i = 0; i < CM2_LUT_SIZE; ++i) { > - u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16 > - | drm_color_lut_extract(drm_lut[i].green, 8) << 8 > - | drm_color_lut_extract(drm_lut[i].blue, 8); > + if (rcmm->lut.enabled != enable) { > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, > + enable ? CM2_LUT_CTRL_LUT_EN : 0); > + rcmm->lut.enabled = enable; > + } > > - rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); > + if (table) { > + unsigned int i; > + > + /* > + * Scale the DRM LUT table entries to the hardware precision > + * and program it. > + */ > + for (i = 0; i < CM2_LUT_SIZE; ++i) { > + const struct drm_color_lut *lut = &table[i]; > + > + u32 entry = drm_color_lut_extract(lut->red, 8) << 16 > + | drm_color_lut_extract(lut->green, 8) << 8 > + | drm_color_lut_extract(lut->blue, 8); > + > + rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); > + } > } > } > > @@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev, > { > struct rcar_cmm *rcmm = platform_get_drvdata(pdev); > > - /* Disable LUT if no table is provided. */ > - if (!config->lut.table) { > - if (rcmm->lut.enabled) { > - rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); > - rcmm->lut.enabled = false; > - } > - > - return 0; > - } > - > - /* Enable LUT and program the new gamma table values. */ > - if (!rcmm->lut.enabled) { > - rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); > - rcmm->lut.enabled = true; > - } > - > - rcar_cmm_lut_write(rcmm, config->lut.table); > + if (config->lut.update) > + rcar_cmm_lut_configure(rcmm, config->lut.table); Does something need to reset config->lut.update to false? Or is this structure reset / cleaned on each call? Never mind, looks like this is always used from a fresh initialised structure in rcar_du_cmm_setup(). > > return 0; > } > @@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev) > { > struct rcar_cmm *rcmm = platform_get_drvdata(pdev); > > - rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); > - rcmm->lut.enabled = false; > + rcar_cmm_lut_configure(rcmm, NULL); > > pm_runtime_put(&pdev->dev); > } > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h > index b5f7ec6db04a..f4b16535ec16 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h > @@ -13,16 +13,23 @@ > struct drm_color_lut; > struct platform_device; > > +/** > + * struct rcar_cmm_table_config - CMM LUT configuration > + * @update: When true, update the LUT configuration. > + * @table: Table data. The LUT is enabled if non-NULL, and disabled > + * otherwise. The value is ignored if @update is false. > + */ > +struct rcar_cmm_table_config { > + bool update; > + struct drm_color_lut *table; > +}; > + > /** > * struct rcar_cmm_config - CMM configuration > - * > - * @lut: 1D-LUT configuration > - * @lut.table: 1D-LUT table entries. Disable LUT operations when NULL > + * @lut: 1D-LUT configuration > */ > struct rcar_cmm_config { > - struct { > - struct drm_color_lut *table; > - } lut; > + struct rcar_cmm_table_config lut; > }; > > #if IS_ENABLED(CONFIG_DRM_RCAR_CMM) > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > index 9a099c0fe1d4..426b1870b3cb 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, > return 0; > } > > -static void rcar_du_cmm_setup(struct drm_crtc *crtc) > +static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc, > + const struct drm_crtc_state *old_state, > + const struct drm_crtc_state *new_state) > { > - struct drm_property_blob *drm_lut = crtc->state->gamma_lut; > - struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > struct rcar_cmm_config cmm_config = {}; > > if (!rcrtc->cmm) > return; > > - if (drm_lut) > - cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data; > + if (!old_state || > + !old_state->gamma_lut != !new_state->gamma_lut || > + (old_state->gamma_lut && new_state->gamma_lut && > + old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) { The conditional looks a bit terse, but it looks like it does the expected. Everything else looks good to me. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > + cmm_config.lut.update = true; > + cmm_config.lut.table = new_state->gamma_lut > + ? new_state->gamma_lut->data : NULL; > + } > > rcar_cmm_setup(rcrtc->cmm, &cmm_config); > } > @@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, > * after the DU channel has been activated. Investigate the impact > * of this restriction on the first displayed frame. > */ > - rcar_du_cmm_setup(crtc); > + rcar_du_cmm_setup(rcrtc, NULL, crtc->state); > } > > static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, > @@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, > static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, > struct drm_atomic_state *state) > { > + struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state, > + crtc); > struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > > WARN_ON(!crtc->state->enable); > @@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, > > /* If the active state changed, we let .atomic_enable handle CMM. */ > if (crtc->state->color_mgmt_changed && !crtc->state->active_changed) > - rcar_du_cmm_setup(crtc); > + rcar_du_cmm_setup(rcrtc, old_state, crtc->state); > > if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) > rcar_du_vsp_atomic_begin(rcrtc); >
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c index 382d53f8a22e..ccc8c8b03bac 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) iowrite32(data, rcmm->base + reg); } -/* - * rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision - * and write to the CMM registers - * @rcmm: Pointer to the CMM device - * @drm_lut: Pointer to the DRM LUT table - */ -static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, - const struct drm_color_lut *drm_lut) +static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm, + const struct drm_color_lut *table) { - unsigned int i; + bool enable = !!table; - for (i = 0; i < CM2_LUT_SIZE; ++i) { - u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16 - | drm_color_lut_extract(drm_lut[i].green, 8) << 8 - | drm_color_lut_extract(drm_lut[i].blue, 8); + if (rcmm->lut.enabled != enable) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, + enable ? CM2_LUT_CTRL_LUT_EN : 0); + rcmm->lut.enabled = enable; + } - rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); + if (table) { + unsigned int i; + + /* + * Scale the DRM LUT table entries to the hardware precision + * and program it. + */ + for (i = 0; i < CM2_LUT_SIZE; ++i) { + const struct drm_color_lut *lut = &table[i]; + + u32 entry = drm_color_lut_extract(lut->red, 8) << 16 + | drm_color_lut_extract(lut->green, 8) << 8 + | drm_color_lut_extract(lut->blue, 8); + + rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); + } } } @@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev, { struct rcar_cmm *rcmm = platform_get_drvdata(pdev); - /* Disable LUT if no table is provided. */ - if (!config->lut.table) { - if (rcmm->lut.enabled) { - rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); - rcmm->lut.enabled = false; - } - - return 0; - } - - /* Enable LUT and program the new gamma table values. */ - if (!rcmm->lut.enabled) { - rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); - rcmm->lut.enabled = true; - } - - rcar_cmm_lut_write(rcmm, config->lut.table); + if (config->lut.update) + rcar_cmm_lut_configure(rcmm, config->lut.table); return 0; } @@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev) { struct rcar_cmm *rcmm = platform_get_drvdata(pdev); - rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); - rcmm->lut.enabled = false; + rcar_cmm_lut_configure(rcmm, NULL); pm_runtime_put(&pdev->dev); } diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h index b5f7ec6db04a..f4b16535ec16 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -13,16 +13,23 @@ struct drm_color_lut; struct platform_device; +/** + * struct rcar_cmm_table_config - CMM LUT configuration + * @update: When true, update the LUT configuration. + * @table: Table data. The LUT is enabled if non-NULL, and disabled + * otherwise. The value is ignored if @update is false. + */ +struct rcar_cmm_table_config { + bool update; + struct drm_color_lut *table; +}; + /** * struct rcar_cmm_config - CMM configuration - * - * @lut: 1D-LUT configuration - * @lut.table: 1D-LUT table entries. Disable LUT operations when NULL + * @lut: 1D-LUT configuration */ struct rcar_cmm_config { - struct { - struct drm_color_lut *table; - } lut; + struct rcar_cmm_table_config lut; }; #if IS_ENABLED(CONFIG_DRM_RCAR_CMM) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 9a099c0fe1d4..426b1870b3cb 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, return 0; } -static void rcar_du_cmm_setup(struct drm_crtc *crtc) +static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc, + const struct drm_crtc_state *old_state, + const struct drm_crtc_state *new_state) { - struct drm_property_blob *drm_lut = crtc->state->gamma_lut; - struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct rcar_cmm_config cmm_config = {}; if (!rcrtc->cmm) return; - if (drm_lut) - cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data; + if (!old_state || + !old_state->gamma_lut != !new_state->gamma_lut || + (old_state->gamma_lut && new_state->gamma_lut && + old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) { + cmm_config.lut.update = true; + cmm_config.lut.table = new_state->gamma_lut + ? new_state->gamma_lut->data : NULL; + } rcar_cmm_setup(rcrtc->cmm, &cmm_config); } @@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, * after the DU channel has been activated. Investigate the impact * of this restriction on the first displayed frame. */ - rcar_du_cmm_setup(crtc); + rcar_du_cmm_setup(rcrtc, NULL, crtc->state); } static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state, + crtc); struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); WARN_ON(!crtc->state->enable); @@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, /* If the active state changed, we let .atomic_enable handle CMM. */ if (crtc->state->color_mgmt_changed && !crtc->state->active_changed) - rcar_du_cmm_setup(crtc); + rcar_du_cmm_setup(rcrtc, old_state, crtc->state); if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
To prepare for CLU support, expend the CMM API exposed to the DU driver to separate the LUT table pointer from the LUT update decision. This will be required, as we will need to update the LUT and CLU independently. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_cmm.c | 60 ++++++++++++-------------- drivers/gpu/drm/rcar-du/rcar_cmm.h | 19 +++++--- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++--- 3 files changed, 55 insertions(+), 46 deletions(-)