Re: [PATCH 1/4] drm: rcar-du: cmm: Refactor LUT configuration

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);