Re: [PATCH v6 06/10] drm/i915/dsb: functions to enable/disable DSB engine.

["Sharma, Shashank" <shashank.sharma@intel.com>]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> DSB will be used for performance improvement for some special scenario.
> DSB engine will be enabled based on need and after completion of its work
> will be disabled. Api added for enable/disable operation by using DSB_CTRL
> register.
>
> v1: Initial version.
> v2: POSTING_READ added after writing control register. (Shashank)
> v3: cosmetic changes done. (Shashank)
>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 40 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h          |  1 +
>   2 files changed, 41 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index b1da2b06263a..2b0ffc0afb74 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -26,6 +26,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
>   	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
>   }
>   
> +static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 dsb_ctrl;
> +
> +	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
> +	if (DSB_STATUS & dsb_ctrl) {
> +		DRM_DEBUG_KMS("DSB engine is busy.\n");
> +		return false;
> +	}
> +
> +	dsb_ctrl |= DSB_ENABLE;
> +	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
> +
> +	POSTING_READ(DSB_CTRL(pipe, dsb->id));
> +	return true;
> +}
> +
> +static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 dsb_ctrl;
> +
> +	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
> +	if (DSB_STATUS & dsb_ctrl) {
> +		DRM_DEBUG_KMS("DSB engine is busy.\n");
> +		return false;
> +	}
> +
> +	dsb_ctrl &= ~DSB_ENABLE;

Do we really need to care about reading the reg val first and then 
disabling it ? I can understand that for enable().

How about this:

if (!dsb_is_busy()) {

     I915_WRITE(DSB_CTRL(pipe, dsb->id), 0);

    POSTING_READ();

    return true;

}

DRM_DEBUG_KMS("DSB engine is busy.\n");
return false;

But this is optional suggestion, you can take a call on this.

> +	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
> +
> +	POSTING_READ(DSB_CTRL(pipe, dsb->id));
> +	return true;
> +}
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9188a0b53538..2dbaa49f5c74 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11688,6 +11688,7 @@ enum skl_power_gate {
>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>   					 (pipe) * 0x1000 + (id) * 100)
>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
> +#define   DSB_ENABLE			(1 << 31)
>   #define   DSB_STATUS			(1 << 0)
>   

With or without suggested change above: Feel free to use

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

>   #endif /* _I915_REG_H_ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx