Re: [PATCH v2 14/15] drm/i915: Tweak BIOS fb reuse check

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 15.12.2023 11:59, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Currently we assume that we bind the BIOS fb exactly into the same
> ggtt address where the BIOS left it. That is about to change, and
> in order to keep intel_reuse_initial_plane_obj() working as intended
> we need to compare the original ggtt offset (called 'base' here)
> as opposed ot the actual vma ggtt offset we selected. Otherwise

s/ot/to/

> the first plane could change the ggtt offset, and then subsequent
> planes would no longer notice that they are in fact using the same
> ggtt offset that the first plane was already using. Thus the reuse
> check will fail and we proceed to turn off these subsequent planes.
>
> TODO: would probably make more sense to do the pure readout first
> for all the planes, then check for fb reuse, and only then proceed
> to pin the object into the final location in the ggtt...
>
> Cc: Paz Zcharya <pazz@xxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>


Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>

Regards
Andrzej

> ---
>   .../drm/i915/display/intel_plane_initial.c    | 34 +++++++++++--------
>   1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index b7e12b60d68b..82ab98985a09 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -13,20 +13,21 @@
>   #include "intel_plane_initial.h"
>   
>   static bool
> -intel_reuse_initial_plane_obj(struct drm_i915_private *i915,
> -			      const struct intel_initial_plane_config *plane_config,
> +intel_reuse_initial_plane_obj(struct intel_crtc *this,
> +			      const struct intel_initial_plane_config plane_configs[],
>   			      struct drm_framebuffer **fb,
>   			      struct i915_vma **vma)
>   {
> +	struct drm_i915_private *i915 = to_i915(this->base.dev);
>   	struct intel_crtc *crtc;
>   
>   	for_each_intel_crtc(&i915->drm, crtc) {
> -		struct intel_crtc_state *crtc_state =
> -			to_intel_crtc_state(crtc->base.state);
> -		struct intel_plane *plane =
> +		const struct intel_plane *plane =
>   			to_intel_plane(crtc->base.primary);
> -		struct intel_plane_state *plane_state =
> +		const struct intel_plane_state *plane_state =
>   			to_intel_plane_state(plane->base.state);
> +		const struct intel_crtc_state *crtc_state =
> +			to_intel_crtc_state(crtc->base.state);
>   
>   		if (!crtc_state->uapi.active)
>   			continue;
> @@ -34,7 +35,7 @@ intel_reuse_initial_plane_obj(struct drm_i915_private *i915,
>   		if (!plane_state->ggtt_vma)
>   			continue;
>   
> -		if (intel_plane_ggtt_offset(plane_state) == plane_config->base) {
> +		if (plane_configs[this->pipe].base == plane_configs[crtc->pipe].base) {
>   			*fb = plane_state->hw.fb;
>   			*vma = plane_state->ggtt_vma;
>   			return true;
> @@ -265,10 +266,11 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
>   
>   static void
>   intel_find_initial_plane_obj(struct intel_crtc *crtc,
> -			     struct intel_initial_plane_config *plane_config)
> +			     struct intel_initial_plane_config plane_configs[])
>   {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_initial_plane_config *plane_config =
> +		&plane_configs[crtc->pipe];
>   	struct intel_plane *plane =
>   		to_intel_plane(crtc->base.primary);
>   	struct intel_plane_state *plane_state =
> @@ -294,7 +296,7 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>   	 * Failed to alloc the obj, check to see if we should share
>   	 * an fb with another CRTC instead
>   	 */
> -	if (intel_reuse_initial_plane_obj(dev_priv, plane_config, &fb, &vma))
> +	if (intel_reuse_initial_plane_obj(crtc, plane_configs, &fb, &vma))
>   		goto valid_fb;
>   
>   	/*
> @@ -359,10 +361,12 @@ static void plane_config_fini(struct intel_initial_plane_config *plane_config)
>   
>   void intel_initial_plane_config(struct drm_i915_private *i915)
>   {
> +	struct intel_initial_plane_config plane_configs[I915_MAX_PIPES] = {};
>   	struct intel_crtc *crtc;
>   
>   	for_each_intel_crtc(&i915->drm, crtc) {
> -		struct intel_initial_plane_config plane_config = {};
> +		struct intel_initial_plane_config *plane_config =
> +			&plane_configs[crtc->pipe];
>   
>   		if (!to_intel_crtc_state(crtc->base.state)->uapi.active)
>   			continue;
> @@ -374,14 +378,14 @@ void intel_initial_plane_config(struct drm_i915_private *i915)
>   		 * can even allow for smooth boot transitions if the BIOS
>   		 * fb is large enough for the active pipe configuration.
>   		 */
> -		i915->display.funcs.display->get_initial_plane_config(crtc, &plane_config);
> +		i915->display.funcs.display->get_initial_plane_config(crtc, plane_config);
>   
>   		/*
>   		 * If the fb is shared between multiple heads, we'll
>   		 * just get the first one.
>   		 */
> -		intel_find_initial_plane_obj(crtc, &plane_config);
> +		intel_find_initial_plane_obj(crtc, plane_configs);
>   
> -		plane_config_fini(&plane_config);
> +		plane_config_fini(plane_config);
>   	}
>   }