Re: [Intel-gfx] [PATCH 12/21] drm/i915: constify fdi link training vtable

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Dave Airlie <airlied@gmail.com>, intel-gfx@lists.freedesktop.org
Cc: Dave Airlie <airlied@redhat.com>
Subject: Re: [Intel-gfx] [PATCH 12/21] drm/i915: constify fdi link training vtable
Date: Wed, 08 Sep 2021 15:03:42 +0300	[thread overview]
Message-ID: <87tuiv46ld.fsf@intel.com> (raw)
In-Reply-To: <20210908003944.2972024-13-airlied@gmail.com>

On Wed, 08 Sep 2021, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> Avoid having writeable function pointers.
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_fdi.c     | 18 +++++++++++++++---
>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
>  3 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 87950202f4ce..0ad577aceb9d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2100,7 +2100,7 @@ static void ilk_pch_enable(const struct intel_atomic_state *state,
>  	assert_pch_transcoder_disabled(dev_priv, pipe);
>  
>  	/* For PCH output, training FDI link */
> -	dev_priv->fdi_funcs.fdi_link_train(crtc, crtc_state);
> +	dev_priv->fdi_funcs->fdi_link_train(crtc, crtc_state);
>  
>  	/* We need to program the right clock selection before writing the pixel
>  	 * mutliplier into the DPLL. */
> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
> index d9f952e0c67f..68aa9c7b18ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_fdi.c
> +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
> @@ -1005,15 +1005,27 @@ void lpt_fdi_program_mphy(struct drm_i915_private *dev_priv)
>  	intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
>  }
>  
> +static const struct drm_i915_fdi_link_train_funcs ilk_funcs = {
> +	.fdi_link_train = ilk_fdi_link_train

Oh, I guess we could add , at the end of all of these, across all
patches, even if some of them currently hold only one member. It's just
so much cleaner if ever you need to add another member.

BR,
Jani.

> +};
> +
> +static const struct drm_i915_fdi_link_train_funcs gen6_funcs = {
> +	.fdi_link_train = gen6_fdi_link_train
> +};
> +
> +static const struct drm_i915_fdi_link_train_funcs ivb_funcs = {
> +	.fdi_link_train = ivb_manual_fdi_link_train
> +};
> +
>  void
>  intel_fdi_init_hook(struct drm_i915_private *dev_priv)
>  {
>  	if (IS_IRONLAKE(dev_priv)) {
> -		dev_priv->fdi_funcs.fdi_link_train = ilk_fdi_link_train;
> +		dev_priv->fdi_funcs = &ilk_funcs;
>  	} else if (IS_SANDYBRIDGE(dev_priv)) {
> -		dev_priv->fdi_funcs.fdi_link_train = gen6_fdi_link_train;
> +		dev_priv->fdi_funcs = &gen6_funcs;
>  	} else if (IS_IVYBRIDGE(dev_priv)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
> -		dev_priv->fdi_funcs.fdi_link_train = ivb_manual_fdi_link_train;
> +		dev_priv->fdi_funcs = &ivb_funcs;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 461ab0a0f088..b3765222e717 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1006,7 +1006,7 @@ struct drm_i915_private {
>  	struct drm_i915_irq_funcs irq_funcs;
>  
>  	/* fdi display functions */
> -	struct drm_i915_fdi_link_train_funcs fdi_funcs;
> +	const struct drm_i915_fdi_link_train_funcs *fdi_funcs;
>  
>  	/* display pll funcs */
>  	struct drm_i915_dpll_funcs dpll_funcs;

-- 
Jani Nikula, Intel Open Source Graphics Center