On Tue, 17 Jun 2025, Suraj Kandpal <suraj.kand...@intel.com> wrote:
> We only support resolution up to 4k for single pipe when using
> YUV420 format so we prune these modes and restrict the plane size
> at src. This is because pipe scaling will not support YUV420 scaling
> for hwidth > 4096.
>
> --v2
> -Use output format to check [Ville]
> -Add Bspec references
> -Modify commit messge to point to why this is needed
>
> --v3
> -Use a function skl_scaler_mode_valid which is routed throug
> intel_pfit_mode_valid [Ville]
> -Combine the check conditons [Jonathan]
>
> Bspec: 49247, 50441
> Signed-off-by: Suraj Kandpal <suraj.kand...@intel.com>
> Reviewed-by: Jonathan Cavitt <jonathan.cav...@intel.com> #v2
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 10 +++++++---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 3 +++
> drivers/gpu/drm/i915/display/intel_pfit.c | 12 ++++++++++++
> drivers/gpu/drm/i915/display/intel_pfit.h | 12 +++++++++++-
> drivers/gpu/drm/i915/display/skl_scaler.c | 18 ++++++++++++++++++
> drivers/gpu/drm/i915/display/skl_scaler.h | 12 ++++++++++++
> 6 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 277b40b13948..7af936e6ea46 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1418,6 +1418,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> struct intel_display *display = to_intel_display(_connector->dev);
> struct intel_connector *connector = to_intel_connector(_connector);
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> + enum intel_output_format sink_format, output_format;
> const struct drm_display_mode *fixed_mode;
> int target_clock = mode->clock;
> int max_rate, mode_rate, max_lanes, max_link_clock;
> @@ -1451,6 +1452,12 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> mode->hdisplay,
> target_clock);
> max_dotclk *= num_joined_pipes;
>
> + sink_format = intel_dp_sink_format(connector, mode);
> + output_format = intel_dp_output_format(connector, sink_format);
> +
> + if (!intel_pfit_mode_valid(display, mode, output_format,
> num_joined_pipes, &status))
> + return status;
I think all _mode_valid() functions should return enum drm_mode_status.
This now returns pretty much the same thing in two different ways, and
leaves status unset if it returns true. That's not a great interface.
> +
> if (target_clock > max_dotclk)
> return MODE_CLOCK_HIGH;
>
> @@ -1466,11 +1473,8 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>
> intel_dp_mode_min_output_bpp(connector, mode));
>
> if (intel_dp_has_dsc(connector)) {
> - enum intel_output_format sink_format, output_format;
> int pipe_bpp;
>
> - sink_format = intel_dp_sink_format(connector, mode);
> - output_format = intel_dp_output_format(connector, sink_format);
> /*
> * TBD pass the connector BPC,
> * for now U8_MAX so that max BPC on that platform would be
> picked
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 9961ff259298..53ba1e16100d 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2053,6 +2053,9 @@ intel_hdmi_mode_valid(struct drm_connector *_connector,
> else
> sink_format = INTEL_OUTPUT_FORMAT_RGB;
>
> + if (!intel_pfit_mode_valid(display, mode, sink_format, 0, &status))
> + return status;
> +
> status = intel_hdmi_mode_clock_valid(&connector->base, clock,
> has_hdmi_sink, sink_format);
> if (status != MODE_OK) {
> if (ycbcr_420_only ||
> diff --git a/drivers/gpu/drm/i915/display/intel_pfit.c
> b/drivers/gpu/drm/i915/display/intel_pfit.c
> index 13541be4d6df..a8fb92c3417a 100644
> --- a/drivers/gpu/drm/i915/display/intel_pfit.c
> +++ b/drivers/gpu/drm/i915/display/intel_pfit.c
> @@ -14,6 +14,7 @@
> #include "intel_lvds_regs.h"
> #include "intel_pfit.h"
> #include "intel_pfit_regs.h"
> +#include "skl_scaler.h"
>
> static int intel_pch_pfit_check_dst_window(const struct intel_crtc_state
> *crtc_state)
> {
> @@ -546,6 +547,17 @@ static int gmch_panel_fitting(struct intel_crtc_state
> *crtc_state,
> return intel_gmch_pfit_check_timings(crtc_state);
> }
>
> +bool
> +intel_pfit_mode_valid(struct intel_display *display,
> + const struct drm_display_mode *mode,
> + enum intel_output_format output_format,
> + int num_joined_pipes,
> + enum drm_mode_status *status)
> +{
> + return skl_scaler_mode_valid(display, mode, output_format,
> + num_joined_pipes, status);
> +}
> +
Please explain why we're adding the extra dummy layer. Do we expect more
to be added here?
BR,
Jani.
> int intel_pfit_compute_config(struct intel_crtc_state *crtc_state,
> const struct drm_connector_state *conn_state)
> {
> diff --git a/drivers/gpu/drm/i915/display/intel_pfit.h
> b/drivers/gpu/drm/i915/display/intel_pfit.h
> index ef34f9b49d09..ce68ceebc700 100644
> --- a/drivers/gpu/drm/i915/display/intel_pfit.h
> +++ b/drivers/gpu/drm/i915/display/intel_pfit.h
> @@ -6,8 +6,14 @@
> #ifndef __INTEL_PFIT_H__
> #define __INTEL_PFIT_H__
>
> +#include <linux/types.h>
> +
> +enum drm_mode_status;
> +struct drm_display_mode;
> struct drm_connector_state;
> struct intel_crtc_state;
> +struct intel_display;
> +enum intel_output_format;
>
> int intel_pfit_compute_config(struct intel_crtc_state *crtc_state,
> const struct drm_connector_state *conn_state);
> @@ -17,5 +23,9 @@ void ilk_pfit_get_config(struct intel_crtc_state
> *crtc_state);
> void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state);
> void i9xx_pfit_disable(const struct intel_crtc_state *old_crtc_state);
> void i9xx_pfit_get_config(struct intel_crtc_state *crtc_state);
> -
> +bool intel_pfit_mode_valid(struct intel_display *display,
> + const struct drm_display_mode *mode,
> + enum intel_output_format output_format,
> + int num_joined_pipes,
> + enum drm_mode_status *status);
> #endif /* __INTEL_PFIT_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c
> b/drivers/gpu/drm/i915/display/skl_scaler.c
> index d77798499c57..27001b52daf8 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -134,6 +134,24 @@ static void skl_scaler_max_dst_size(struct intel_crtc
> *crtc,
> }
> }
>
> +bool
> +skl_scaler_mode_valid(struct intel_display *display,
> + const struct drm_display_mode *mode,
> + enum intel_output_format output_format,
> + int num_joined_pipes,
> + enum drm_mode_status *status)
> +{
> + if (num_joined_pipes < 2 && output_format ==
> INTEL_OUTPUT_FORMAT_YCBCR420) {
> + if (DISPLAY_VER(display) >= 14 &&
> + mode->hdisplay > 4096) {
> + *status = MODE_NO_420;
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> static int
> skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
> unsigned int scaler_user, int *scaler_id,
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.h
> b/drivers/gpu/drm/i915/display/skl_scaler.h
> index 355ea15260ca..37c6543b4731 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.h
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.h
> @@ -5,10 +5,16 @@
> #ifndef INTEL_SCALER_H
> #define INTEL_SCALER_H
>
> +#include <linux/types.h>
> +
> +enum drm_mode_status;
> +struct drm_display_mode;
> struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> +struct intel_display;
> struct intel_dsb;
> +enum intel_output_format;
> struct intel_plane;
> struct intel_plane_state;
>
> @@ -32,4 +38,10 @@ void skl_scaler_disable(const struct intel_crtc_state
> *old_crtc_state);
>
> void skl_scaler_get_config(struct intel_crtc_state *crtc_state);
>
> +bool skl_scaler_mode_valid(struct intel_display *display,
> + const struct drm_display_mode *mode,
> + enum intel_output_format output_format,
> + int num_joined_pipes,
> + enum drm_mode_status *status);
> +
> #endif
--
Jani Nikula, Intel
