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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxx>

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

-- 
Regards
--
Kieran



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux