diff mbox series

[2/2] drm/i915/display: Call panel_fitting from pipe_config

Message ID 20240808045407.2365733-1-nemesa.garg@intel.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Nemesa Garg Aug. 8, 2024, 4:54 a.m. UTC
In panel fitter/pipe scaler scenario the pch_pfit configuration
currently takes place before accounting for pipe_src width for
joiner. This causes issue when pch_pfit and joiner get enabled
together.

Introduce a new boolean flag need_joiner which is set during dp
compute_config in joiner case and later is used to compute
panel_fitting in pipe_config. Modify pch_panel_fitting to handle
joiner pipes by adjusting crtc_hdisplay accordingly.

v2: Address comments (Ankit)
v3: Change flag name (Ankit)

Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c       | 11 +++++++++++
 drivers/gpu/drm/i915/display/intel_display_types.h |  1 +
 drivers/gpu/drm/i915/display/intel_dp.c            | 11 ++++++++---
 drivers/gpu/drm/i915/display/intel_panel.c         |  3 +++
 4 files changed, 23 insertions(+), 3 deletions(-)

Comments

Ankit Nautiyal Aug. 13, 2024, 5:18 a.m. UTC | #1
On 8/8/2024 10:24 AM, Nemesa Garg wrote:
> In panel fitter/pipe scaler scenario the pch_pfit configuration
> currently takes place before accounting for pipe_src width for
> joiner. This causes issue when pch_pfit and joiner get enabled
> together.
>
> Introduce a new boolean flag need_joiner which is set during dp
> compute_config in joiner case and later is used to compute
> panel_fitting in pipe_config. Modify pch_panel_fitting to handle
> joiner pipes by adjusting crtc_hdisplay accordingly.
>
> v2: Address comments (Ankit)

In general, it would be good to document the changes patch went through.


> v3: Change flag name (Ankit)
>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_display.c       | 11 +++++++++++
>   drivers/gpu/drm/i915/display/intel_display_types.h |  1 +
>   drivers/gpu/drm/i915/display/intel_dp.c            | 11 ++++++++---
>   drivers/gpu/drm/i915/display/intel_panel.c         |  3 +++
>   4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 8bbde03f2508..82b67c0a90e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4796,6 +4796,17 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
>   		return ret;
>   	}
>   
> +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> +		if (connector_state->crtc != &crtc->base)
> +			continue;
> +
> +		if (crtc_state->pch_pfit.need_joiner) {
> +			ret = intel_panel_fitting(crtc_state, connector_state);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
>   	/* Dithering seems to not pass-through bits correctly when it should, so
>   	 * only enable it on 6bpc panels and when its not a compliance
>   	 * test requesting 6bpc video pattern.
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index a04d52dbf6e1..eb9713b088c6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1258,6 +1258,7 @@ struct intel_crtc_state {
>   		struct drm_rect dst;
>   		bool enabled;
>   		bool force_thru;
> +		bool need_joiner;
>   	} pch_pfit;
>   
>   	/* FDI configuration, only valid if has_pch_encoder is set. */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 65182bf69b62..d5d9d4f21fc7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2953,9 +2953,14 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   
>   	if ((intel_dp_is_edp(intel_dp) && fixed_mode) ||
>   	    pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
> -		ret = intel_panel_fitting(pipe_config, conn_state);
> -		if (ret)
> -			return ret;
> +		if (!pipe_config->joiner_pipes) {
> +			ret = intel_panel_fitting(pipe_config, conn_state);
> +			if (ret)
> +				return ret;
> +		} else {
> +			 /* Incase of joiner panel_fitting is handled during pipe_config */

'pipe_config' seems confusing, as we already have a variable pipe_config.

Also, perhaps we can explain a bit more about the rationale behind 
handling this at later point.

Apart from the minor things above, the patch looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> +			pipe_config->pch_pfit.need_joiner = true;
> +		}
>   	}
>   
>   	pipe_config->limited_color_range =
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index dd18136d1c61..0da45c2330d3 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -395,6 +395,9 @@ static int pch_panel_fitting(struct intel_crtc_state *crtc_state,
>   	u16 crtc_hdisplay = adjusted_mode->crtc_hdisplay;
>   	u16 crtc_vdisplay = adjusted_mode->crtc_vdisplay;
>   
> +	if (crtc_state->joiner_pipes)
> +		crtc_hdisplay = adjusted_mode->crtc_hdisplay / 2;
> +
>   	/* Native modes don't need fitting */
>   	if (crtc_hdisplay == pipe_src_w &&
>   	    crtc_vdisplay == pipe_src_h &&
Jani Nikula Aug. 13, 2024, 7:52 a.m. UTC | #2
On Thu, 08 Aug 2024, Nemesa Garg <nemesa.garg@intel.com> wrote:
> In panel fitter/pipe scaler scenario the pch_pfit configuration
> currently takes place before accounting for pipe_src width for
> joiner. This causes issue when pch_pfit and joiner get enabled
> together.
>
> Introduce a new boolean flag need_joiner which is set during dp
> compute_config in joiner case and later is used to compute
> panel_fitting in pipe_config. Modify pch_panel_fitting to handle
> joiner pipes by adjusting crtc_hdisplay accordingly.

So I still don't like the fact that intel_panel_fitting() is called in
different ways for different connectors, controlled by a flag in crtc
state.

That said, I couldn't come up with a better idea either, apart from
moving *all* panel fitting intel_modeset_pipe_config().

Cc: Ville, in case he has some ideas for this. Please hold off on
merging until we get some input from him.


Thanks,
Jani.


>
> v2: Address comments (Ankit)
> v3: Change flag name (Ankit)
>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c       | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_display_types.h |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c            | 11 ++++++++---
>  drivers/gpu/drm/i915/display/intel_panel.c         |  3 +++
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 8bbde03f2508..82b67c0a90e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4796,6 +4796,17 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
>  		return ret;
>  	}
>  
> +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> +		if (connector_state->crtc != &crtc->base)
> +			continue;
> +
> +		if (crtc_state->pch_pfit.need_joiner) {
> +			ret = intel_panel_fitting(crtc_state, connector_state);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
>  	/* Dithering seems to not pass-through bits correctly when it should, so
>  	 * only enable it on 6bpc panels and when its not a compliance
>  	 * test requesting 6bpc video pattern.
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index a04d52dbf6e1..eb9713b088c6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1258,6 +1258,7 @@ struct intel_crtc_state {
>  		struct drm_rect dst;
>  		bool enabled;
>  		bool force_thru;
> +		bool need_joiner;
>  	} pch_pfit;
>  
>  	/* FDI configuration, only valid if has_pch_encoder is set. */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 65182bf69b62..d5d9d4f21fc7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2953,9 +2953,14 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  
>  	if ((intel_dp_is_edp(intel_dp) && fixed_mode) ||
>  	    pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
> -		ret = intel_panel_fitting(pipe_config, conn_state);
> -		if (ret)
> -			return ret;
> +		if (!pipe_config->joiner_pipes) {
> +			ret = intel_panel_fitting(pipe_config, conn_state);
> +			if (ret)
> +				return ret;
> +		} else {
> +			 /* Incase of joiner panel_fitting is handled during pipe_config */
> +			pipe_config->pch_pfit.need_joiner = true;
> +		}
>  	}
>  
>  	pipe_config->limited_color_range =
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index dd18136d1c61..0da45c2330d3 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -395,6 +395,9 @@ static int pch_panel_fitting(struct intel_crtc_state *crtc_state,
>  	u16 crtc_hdisplay = adjusted_mode->crtc_hdisplay;
>  	u16 crtc_vdisplay = adjusted_mode->crtc_vdisplay;
>  
> +	if (crtc_state->joiner_pipes)
> +		crtc_hdisplay = adjusted_mode->crtc_hdisplay / 2;
> +
>  	/* Native modes don't need fitting */
>  	if (crtc_hdisplay == pipe_src_w &&
>  	    crtc_vdisplay == pipe_src_h &&
Nemesa Garg Sept. 9, 2024, 6:21 a.m. UTC | #3
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Tuesday, August 13, 2024 1:22 PM
> To: Garg, Nemesa <nemesa.garg@intel.com>; intel-gfx@lists.freedesktop.org;
> Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Garg, Nemesa <nemesa.garg@intel.com>
> Subject: Re: [PATCH 2/2] drm/i915/display: Call panel_fitting from pipe_config
> 
> On Thu, 08 Aug 2024, Nemesa Garg <nemesa.garg@intel.com> wrote:
> > In panel fitter/pipe scaler scenario the pch_pfit configuration
> > currently takes place before accounting for pipe_src width for joiner.
> > This causes issue when pch_pfit and joiner get enabled together.
> >
> > Introduce a new boolean flag need_joiner which is set during dp
> > compute_config in joiner case and later is used to compute
> > panel_fitting in pipe_config. Modify pch_panel_fitting to handle
> > joiner pipes by adjusting crtc_hdisplay accordingly.
> 
> So I still don't like the fact that intel_panel_fitting() is called in different ways for
> different connectors, controlled by a flag in crtc state.
> 
> That said, I couldn't come up with a better idea either, apart from moving *all*
> panel fitting intel_modeset_pipe_config().
> 
> Cc: Ville, in case he has some ideas for this. Please hold off on merging until we
> get some input from him.
> 
> Hi Ville,
Can you please suggest how should I proceed further on this patch.

Thanks and Regards,
Nemesa

> Thanks,
> Jani.
> 
> 
> >
> > v2: Address comments (Ankit)
> > v3: Change flag name (Ankit)
> >
> > Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c       | 11 +++++++++++
> >  drivers/gpu/drm/i915/display/intel_display_types.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_dp.c            | 11 ++++++++---
> >  drivers/gpu/drm/i915/display/intel_panel.c         |  3 +++
> >  4 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 8bbde03f2508..82b67c0a90e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4796,6 +4796,17 @@ intel_modeset_pipe_config(struct
> intel_atomic_state *state,
> >  		return ret;
> >  	}
> >
> > +	for_each_new_connector_in_state(&state->base, connector,
> connector_state, i) {
> > +		if (connector_state->crtc != &crtc->base)
> > +			continue;
> > +
> > +		if (crtc_state->pch_pfit.need_joiner) {
> > +			ret = intel_panel_fitting(crtc_state, connector_state);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +	}
> > +
> >  	/* Dithering seems to not pass-through bits correctly when it should, so
> >  	 * only enable it on 6bpc panels and when its not a compliance
> >  	 * test requesting 6bpc video pattern.
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index a04d52dbf6e1..eb9713b088c6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1258,6 +1258,7 @@ struct intel_crtc_state {
> >  		struct drm_rect dst;
> >  		bool enabled;
> >  		bool force_thru;
> > +		bool need_joiner;
> >  	} pch_pfit;
> >
> >  	/* FDI configuration, only valid if has_pch_encoder is set. */ diff
> > --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 65182bf69b62..d5d9d4f21fc7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2953,9 +2953,14 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> >
> >  	if ((intel_dp_is_edp(intel_dp) && fixed_mode) ||
> >  	    pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> {
> > -		ret = intel_panel_fitting(pipe_config, conn_state);
> > -		if (ret)
> > -			return ret;
> > +		if (!pipe_config->joiner_pipes) {
> > +			ret = intel_panel_fitting(pipe_config, conn_state);
> > +			if (ret)
> > +				return ret;
> > +		} else {
> > +			 /* Incase of joiner panel_fitting is handled during
> pipe_config */
> > +			pipe_config->pch_pfit.need_joiner = true;
> > +		}
> >  	}
> >
> >  	pipe_config->limited_color_range =
> > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> > b/drivers/gpu/drm/i915/display/intel_panel.c
> > index dd18136d1c61..0da45c2330d3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> > @@ -395,6 +395,9 @@ static int pch_panel_fitting(struct intel_crtc_state
> *crtc_state,
> >  	u16 crtc_hdisplay = adjusted_mode->crtc_hdisplay;
> >  	u16 crtc_vdisplay = adjusted_mode->crtc_vdisplay;
> >
> > +	if (crtc_state->joiner_pipes)
> > +		crtc_hdisplay = adjusted_mode->crtc_hdisplay / 2;
> > +
> >  	/* Native modes don't need fitting */
> >  	if (crtc_hdisplay == pipe_src_w &&
> >  	    crtc_vdisplay == pipe_src_h &&
> 
> --
> Jani Nikula, Intel
Ville Syrjälä Sept. 9, 2024, 2:08 p.m. UTC | #4
On Mon, Sep 09, 2024 at 06:21:49AM +0000, Garg, Nemesa wrote:
> 
> 
> > -----Original Message-----
> > From: Jani Nikula <jani.nikula@linux.intel.com>
> > Sent: Tuesday, August 13, 2024 1:22 PM
> > To: Garg, Nemesa <nemesa.garg@intel.com>; intel-gfx@lists.freedesktop.org;
> > Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Garg, Nemesa <nemesa.garg@intel.com>
> > Subject: Re: [PATCH 2/2] drm/i915/display: Call panel_fitting from pipe_config
> > 
> > On Thu, 08 Aug 2024, Nemesa Garg <nemesa.garg@intel.com> wrote:
> > > In panel fitter/pipe scaler scenario the pch_pfit configuration
> > > currently takes place before accounting for pipe_src width for joiner.
> > > This causes issue when pch_pfit and joiner get enabled together.
> > >
> > > Introduce a new boolean flag need_joiner which is set during dp
> > > compute_config in joiner case and later is used to compute
> > > panel_fitting in pipe_config. Modify pch_panel_fitting to handle
> > > joiner pipes by adjusting crtc_hdisplay accordingly.
> > 
> > So I still don't like the fact that intel_panel_fitting() is called in different ways for
> > different connectors, controlled by a flag in crtc state.
> > 
> > That said, I couldn't come up with a better idea either, apart from moving *all*
> > panel fitting intel_modeset_pipe_config().
> > 
> > Cc: Ville, in case he has some ideas for this. Please hold off on merging until we
> > get some input from him.
> > 
> > Hi Ville,
> Can you please suggest how should I proceed further on this patch.

I think we want to move the whole thing to happen after we've computed
final pipe_src and pipe_mode (which can then be used intead of
adjusted_mode in the pfit calculaitons).

The one annoying part of this is that we probably can only do it for the
ilk+ pfit ("pch"), but the old gmch pfit probably has to stay where it
is now because it may have to adjust the actual transcoder timings for
the purposes of adding borders.

Also I don't think we should really need any extra flags in the crtc
state. The pfit code should be able to figure it all out on its own.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 8bbde03f2508..82b67c0a90e0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4796,6 +4796,17 @@  intel_modeset_pipe_config(struct intel_atomic_state *state,
 		return ret;
 	}
 
+	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
+		if (connector_state->crtc != &crtc->base)
+			continue;
+
+		if (crtc_state->pch_pfit.need_joiner) {
+			ret = intel_panel_fitting(crtc_state, connector_state);
+			if (ret)
+				return ret;
+		}
+	}
+
 	/* Dithering seems to not pass-through bits correctly when it should, so
 	 * only enable it on 6bpc panels and when its not a compliance
 	 * test requesting 6bpc video pattern.
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index a04d52dbf6e1..eb9713b088c6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1258,6 +1258,7 @@  struct intel_crtc_state {
 		struct drm_rect dst;
 		bool enabled;
 		bool force_thru;
+		bool need_joiner;
 	} pch_pfit;
 
 	/* FDI configuration, only valid if has_pch_encoder is set. */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 65182bf69b62..d5d9d4f21fc7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2953,9 +2953,14 @@  intel_dp_compute_config(struct intel_encoder *encoder,
 
 	if ((intel_dp_is_edp(intel_dp) && fixed_mode) ||
 	    pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
-		ret = intel_panel_fitting(pipe_config, conn_state);
-		if (ret)
-			return ret;
+		if (!pipe_config->joiner_pipes) {
+			ret = intel_panel_fitting(pipe_config, conn_state);
+			if (ret)
+				return ret;
+		} else {
+			 /* Incase of joiner panel_fitting is handled during pipe_config */
+			pipe_config->pch_pfit.need_joiner = true;
+		}
 	}
 
 	pipe_config->limited_color_range =
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index dd18136d1c61..0da45c2330d3 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -395,6 +395,9 @@  static int pch_panel_fitting(struct intel_crtc_state *crtc_state,
 	u16 crtc_hdisplay = adjusted_mode->crtc_hdisplay;
 	u16 crtc_vdisplay = adjusted_mode->crtc_vdisplay;
 
+	if (crtc_state->joiner_pipes)
+		crtc_hdisplay = adjusted_mode->crtc_hdisplay / 2;
+
 	/* Native modes don't need fitting */
 	if (crtc_hdisplay == pipe_src_w &&
 	    crtc_vdisplay == pipe_src_h &&