Re: [RESEND 5/6] drm/i915/dram: add return value and handling to intel_dram_detect()

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Tue, 2025-05-27 at 12:25 +0300, Jani Nikula wrote:
> We'll want to start returning errors from intel_dram_detect(). As the
> first step, add the return value and error handling, even if we still
> only return 0.
> 
> Do no functional changes, but leave a comment about whether we should
> bail out on dram detection failures.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c      | 4 +++-
>  drivers/gpu/drm/i915/soc/intel_dram.c   | 9 ++++++---
>  drivers/gpu/drm/i915/soc/intel_dram.h   | 2 +-
>  drivers/gpu/drm/xe/display/xe_display.c | 4 +++-
>  4 files changed, 13 insertions(+), 6 deletions(-)
> 

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 3b0bda74697d..20691a85d513 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -568,7 +568,9 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
>  	 * Fill the dram structure to get the system dram info. This will be
>  	 * used for memory latency calculation.
>  	 */
> -	intel_dram_detect(dev_priv);
> +	ret = intel_dram_detect(dev_priv);
> +	if (ret)
> +		goto err_opregion;
>  
>  	intel_bw_init_hw(display);
>  
> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
> index f42dcdb74d40..e7fa938c98cf 100644
> --- a/drivers/gpu/drm/i915/soc/intel_dram.c
> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
> @@ -704,7 +704,7 @@ static int xelpdp_get_dram_info(struct drm_i915_private *i915, struct
> dram_info
>  	return 0;
>  }
>  
> -void intel_dram_detect(struct drm_i915_private *i915)
> +int intel_dram_detect(struct drm_i915_private *i915)
>  {
>  	struct dram_info *dram_info = &i915->dram_info;
>  	int ret;
> @@ -713,7 +713,7 @@ void intel_dram_detect(struct drm_i915_private *i915)
>  	detect_mem_freq(i915);
>  
>  	if (GRAPHICS_VER(i915) < 9 || IS_DG2(i915) || !HAS_DISPLAY(i915))
> -		return;
> +		return 0;
>  
>  	/*
>  	 * Assume level 0 watermark latency adjustment is needed until proven
> @@ -735,8 +735,9 @@ void intel_dram_detect(struct drm_i915_private *i915)
>  	drm_dbg_kms(&i915->drm, "DRAM type: %s\n",
>  		    intel_dram_type_str(dram_info->type));
>  
> +	/* TODO: Do we want to abort probe on dram detection failures? */
>  	if (ret)
> -		return;
> +		return 0;
>  
>  	drm_dbg_kms(&i915->drm, "Num qgv points %u\n", dram_info->num_qgv_points);
>  
> @@ -744,6 +745,8 @@ void intel_dram_detect(struct drm_i915_private *i915)
>  
>  	drm_dbg_kms(&i915->drm, "Watermark level 0 adjustment needed: %s\n",
>  		    str_yes_no(dram_info->wm_lv_0_adjust_needed));
> +
> +	return 0;
>  }
>  
>  const struct dram_info *intel_dram_info(struct drm_device *drm)
> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.h b/drivers/gpu/drm/i915/soc/intel_dram.h
> index 17a20cd2c6d5..25fe60b2b117 100644
> --- a/drivers/gpu/drm/i915/soc/intel_dram.h
> +++ b/drivers/gpu/drm/i915/soc/intel_dram.h
> @@ -11,7 +11,7 @@ struct drm_device;
>  struct dram_info;
>  
>  void intel_dram_edram_detect(struct drm_i915_private *i915);
> -void intel_dram_detect(struct drm_i915_private *i915);
> +int intel_dram_detect(struct drm_i915_private *i915);
>  unsigned int i9xx_fsb_freq(struct drm_i915_private *i915);
>  const struct dram_info *intel_dram_info(struct drm_device *drm);
>  
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 3f92bf51813e..eafe2f093a6c 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -122,7 +122,9 @@ int xe_display_init_early(struct xe_device *xe)
>  	 * Fill the dram structure to get the system dram info. This will be
>  	 * used for memory latency calculation.
>  	 */
> -	intel_dram_detect(xe);
> +	err = intel_dram_detect(xe);
> +	if (err)
> +		goto err_opregion;
>  
>  	intel_bw_init_hw(display);
>