[PATCH] drm/i915/xe3lpd: Prune modes for YUV420
Kandpal, Suraj
suraj.kandpal at intel.com
Tue Jun 17 09:32:20 UTC 2025
> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Tuesday, June 17, 2025 2:30 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Kandpal, Suraj
> <suraj.kandpal at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
> Subject: Re: [PATCH] drm/i915/xe3lpd: Prune modes for YUV420
>
> On Tue, 17 Jun 2025, Suraj Kandpal <suraj.kandpal at 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.kandpal at intel.com>
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at 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.
Sure will fix this
Regards,
Suraj Kandpal
>
> > +
> > 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
More information about the Intel-xe
mailing list