| Message ID | 20190918150707.32420-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Sep 18, 2019 at 06:07:07PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > planes' max resolution. This has the nasty implication that modes on the > connectors' mode list may not be usable when the user asks for a > fullscreen plane. Seeing as that is the most common use case it seems > prudent to filter out modes that don't allow for fullscreen planes to > be enabled. > > Let's do that in the connetor .mode_valid() hook so that normally > such modes are kept hidden but the user is still able to forcibly > specify such a mode if they know they don't need fullscreen planes. > > This is in line with ealier policies regarding certain clock limits. > The idea is to prevent the casual user from encountering a mode that > would fail under typical conditions, but allow the expert user to > force things if they so wish. Isn't this exactly what atomic_check is for? Why not just add a debug message in get_max_plane_size to leave a breadcrumb? Sean > > Maybe in the future we should consider automagically using two > planes when one can't cover the entire screen? Wouldn't be a > great match for the current uapi with explicit planes though, > but I guess no worse than using two pipes (which we apparently > have to in the future anyway). Either that or we'd have to > teach userspace to do it for us. > > v2: Fix icl+ max plane heigth (Manasi) > > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Leho Kraav <leho@kraav.com> > Cc: Sean Paul <sean@poorly.run> > Cc: José Roberto de Souza <jose.souza@intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_display.h | 4 +++ > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dsi.c | 3 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++- > 5 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index f5e65e052d91..0147e4ae3622 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -15753,6 +15753,7 @@ intel_mode_valid(struct drm_device *dev, > DRM_MODE_FLAG_CLKDIV2)) > return MODE_BAD; > > + /* Transcoder timing limits */ > if (INTEL_GEN(dev_priv) >= 11) { > hdisplay_max = 16384; > vdisplay_max = 8192; > @@ -15791,6 +15792,41 @@ intel_mode_valid(struct drm_device *dev, > return MODE_OK; > } > > +enum drm_mode_status > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > + const struct drm_display_mode *mode) > +{ > + int plane_width_max, plane_height_max; > + > + /* > + * intel_mode_valid() should be > + * sufficient on older platforms. > + */ > + if (INTEL_GEN(dev_priv) < 9) > + return MODE_OK; > + > + /* > + * Most people will probably want a fullscreen > + * plane so let's not advertize modes that are > + * too big for that. > + */ > + if (INTEL_GEN(dev_priv) >= 11) { > + plane_width_max = 5120; > + plane_height_max = 4320; > + } else { > + plane_width_max = 5120; > + plane_height_max = 4096; > + } > + > + if (mode->hdisplay > plane_width_max) > + return MODE_H_ILLEGAL; > + > + if (mode->vdisplay > plane_height_max) > + return MODE_V_ILLEGAL; > + > + return MODE_OK; > +} > + > static const struct drm_mode_config_funcs intel_mode_funcs = { > .fb_create = intel_user_framebuffer_create, > .get_format_info = intel_get_format_info, > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > index 66330fcb10d4..b1ae0e59c715 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -32,6 +32,7 @@ enum link_m_n_set; > struct dpll; > struct drm_connector; > struct drm_device; > +struct drm_display_mode; > struct drm_encoder; > struct drm_file; > struct drm_format_info; > @@ -448,6 +449,9 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); > u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, > u32 pixel_format, u64 modifier); > bool intel_plane_can_remap(const struct intel_plane_state *plane_state); > +enum drm_mode_status > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > + const struct drm_display_mode *mode); > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port); > > void intel_plane_destroy(struct drm_plane *plane); > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index d09133a958e1..ccaf9f00b747 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -562,7 +562,7 @@ intel_dp_mode_valid(struct drm_connector *connector, > if (mode->flags & DRM_MODE_FLAG_DBLCLK) > return MODE_H_ILLEGAL; > > - return MODE_OK; > + return intel_mode_valid_max_plane_size(dev_priv, mode); > } > > u32 intel_dp_pack_aux(const u8 *src, int src_bytes) > diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c > index 5fec02aceaed..a2a937109a5a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsi.c > +++ b/drivers/gpu/drm/i915/display/intel_dsi.c > @@ -55,6 +55,7 @@ int intel_dsi_get_modes(struct drm_connector *connector) > enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) > { > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_connector *intel_connector = to_intel_connector(connector); > const struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode; > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > @@ -73,7 +74,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > return MODE_CLOCK_HIGH; > } > > - return MODE_OK; > + return intel_mode_valid_max_plane_size(dev_priv, mode); > } > > struct intel_dsi_host *intel_dsi_host_init(struct intel_dsi *intel_dsi, > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index c500fc9154c8..dbc686515dce 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -2188,8 +2188,10 @@ intel_hdmi_mode_valid(struct drm_connector *connector, > status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, > true, force_dvi); > } > + if (status != MODE_OK) > + return status; > > - return status; > + return intel_mode_valid_max_plane_size(dev_priv, mode); > } > > static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > -- > 2.21.0 >
On Wed, Sep 18, 2019 at 11:24:09AM -0400, Sean Paul wrote: > On Wed, Sep 18, 2019 at 06:07:07PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > > planes' max resolution. This has the nasty implication that modes on the > > connectors' mode list may not be usable when the user asks for a > > fullscreen plane. Seeing as that is the most common use case it seems > > prudent to filter out modes that don't allow for fullscreen planes to > > be enabled. > > > > Let's do that in the connetor .mode_valid() hook so that normally > > such modes are kept hidden but the user is still able to forcibly > > specify such a mode if they know they don't need fullscreen planes. > > > > This is in line with ealier policies regarding certain clock limits. > > The idea is to prevent the casual user from encountering a mode that > > would fail under typical conditions, but allow the expert user to > > force things if they so wish. > > Isn't this exactly what atomic_check is for? Why not just add a debug message in > get_max_plane_size to leave a breadcrumb? There's already a debug message. Won't really help when the screen fails to light up automagically on account of the preferred mode being too big. > > Sean > > > > > Maybe in the future we should consider automagically using two > > planes when one can't cover the entire screen? Wouldn't be a > > great match for the current uapi with explicit planes though, > > but I guess no worse than using two pipes (which we apparently > > have to in the future anyway). Either that or we'd have to > > teach userspace to do it for us. > > > > v2: Fix icl+ max plane heigth (Manasi) > > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Cc: Leho Kraav <leho@kraav.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++++++++++++ > > drivers/gpu/drm/i915/display/intel_display.h | 4 +++ > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > drivers/gpu/drm/i915/display/intel_dsi.c | 3 +- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++- > > 5 files changed, 46 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index f5e65e052d91..0147e4ae3622 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -15753,6 +15753,7 @@ intel_mode_valid(struct drm_device *dev, > > DRM_MODE_FLAG_CLKDIV2)) > > return MODE_BAD; > > > > + /* Transcoder timing limits */ > > if (INTEL_GEN(dev_priv) >= 11) { > > hdisplay_max = 16384; > > vdisplay_max = 8192; > > @@ -15791,6 +15792,41 @@ intel_mode_valid(struct drm_device *dev, > > return MODE_OK; > > } > > > > +enum drm_mode_status > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > + const struct drm_display_mode *mode) > > +{ > > + int plane_width_max, plane_height_max; > > + > > + /* > > + * intel_mode_valid() should be > > + * sufficient on older platforms. > > + */ > > + if (INTEL_GEN(dev_priv) < 9) > > + return MODE_OK; > > + > > + /* > > + * Most people will probably want a fullscreen > > + * plane so let's not advertize modes that are > > + * too big for that. > > + */ > > + if (INTEL_GEN(dev_priv) >= 11) { > > + plane_width_max = 5120; > > + plane_height_max = 4320; > > + } else { > > + plane_width_max = 5120; > > + plane_height_max = 4096; > > + } > > + > > + if (mode->hdisplay > plane_width_max) > > + return MODE_H_ILLEGAL; > > + > > + if (mode->vdisplay > plane_height_max) > > + return MODE_V_ILLEGAL; > > + > > + return MODE_OK; > > +} > > + > > static const struct drm_mode_config_funcs intel_mode_funcs = { > > .fb_create = intel_user_framebuffer_create, > > .get_format_info = intel_get_format_info, > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > > index 66330fcb10d4..b1ae0e59c715 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > @@ -32,6 +32,7 @@ enum link_m_n_set; > > struct dpll; > > struct drm_connector; > > struct drm_device; > > +struct drm_display_mode; > > struct drm_encoder; > > struct drm_file; > > struct drm_format_info; > > @@ -448,6 +449,9 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); > > u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, > > u32 pixel_format, u64 modifier); > > bool intel_plane_can_remap(const struct intel_plane_state *plane_state); > > +enum drm_mode_status > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > + const struct drm_display_mode *mode); > > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port); > > > > void intel_plane_destroy(struct drm_plane *plane); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index d09133a958e1..ccaf9f00b747 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -562,7 +562,7 @@ intel_dp_mode_valid(struct drm_connector *connector, > > if (mode->flags & DRM_MODE_FLAG_DBLCLK) > > return MODE_H_ILLEGAL; > > > > - return MODE_OK; > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > } > > > > u32 intel_dp_pack_aux(const u8 *src, int src_bytes) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c > > index 5fec02aceaed..a2a937109a5a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/display/intel_dsi.c > > @@ -55,6 +55,7 @@ int intel_dsi_get_modes(struct drm_connector *connector) > > enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > struct drm_display_mode *mode) > > { > > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > > struct intel_connector *intel_connector = to_intel_connector(connector); > > const struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode; > > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > > @@ -73,7 +74,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > return MODE_CLOCK_HIGH; > > } > > > > - return MODE_OK; > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > } > > > > struct intel_dsi_host *intel_dsi_host_init(struct intel_dsi *intel_dsi, > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > index c500fc9154c8..dbc686515dce 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > @@ -2188,8 +2188,10 @@ intel_hdmi_mode_valid(struct drm_connector *connector, > > status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, > > true, force_dvi); > > } > > + if (status != MODE_OK) > > + return status; > > > > - return status; > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > } > > > > static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > -- > > 2.21.0 > > > > -- > Sean Paul, Software Engineer, Google / Chromium OS
On Wed, Sep 18, 2019 at 07:02:18PM +0300, Ville Syrjälä wrote: > On Wed, Sep 18, 2019 at 11:24:09AM -0400, Sean Paul wrote: > > On Wed, Sep 18, 2019 at 06:07:07PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > > > planes' max resolution. This has the nasty implication that modes on the > > > connectors' mode list may not be usable when the user asks for a > > > fullscreen plane. Seeing as that is the most common use case it seems > > > prudent to filter out modes that don't allow for fullscreen planes to > > > be enabled. > > > > > > Let's do that in the connetor .mode_valid() hook so that normally > > > such modes are kept hidden but the user is still able to forcibly > > > specify such a mode if they know they don't need fullscreen planes. > > > > > > This is in line with ealier policies regarding certain clock limits. > > > The idea is to prevent the casual user from encountering a mode that > > > would fail under typical conditions, but allow the expert user to > > > force things if they so wish. > > > > Isn't this exactly what atomic_check is for? Why not just add a debug message in > > get_max_plane_size to leave a breadcrumb? > > There's already a debug message. Won't really help when the screen fails > to light up automagically on account of the preferred mode being too > big. > > > > > Sean > > > > > > > > Maybe in the future we should consider automagically using two > > > planes when one can't cover the entire screen? Wouldn't be a > > > great match for the current uapi with explicit planes though, > > > but I guess no worse than using two pipes (which we apparently > > > have to in the future anyway). Either that or we'd have to > > > teach userspace to do it for us. > > > > > > v2: Fix icl+ max plane heigth (Manasi) Thanks for this fix, with this Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > > Cc: Leho Kraav <leho@kraav.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_display.h | 4 +++ > > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_dsi.c | 3 +- > > > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++- > > > 5 files changed, 46 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index f5e65e052d91..0147e4ae3622 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -15753,6 +15753,7 @@ intel_mode_valid(struct drm_device *dev, > > > DRM_MODE_FLAG_CLKDIV2)) > > > return MODE_BAD; > > > > > > + /* Transcoder timing limits */ > > > if (INTEL_GEN(dev_priv) >= 11) { > > > hdisplay_max = 16384; > > > vdisplay_max = 8192; > > > @@ -15791,6 +15792,41 @@ intel_mode_valid(struct drm_device *dev, > > > return MODE_OK; > > > } > > > > > > +enum drm_mode_status > > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > > + const struct drm_display_mode *mode) > > > +{ > > > + int plane_width_max, plane_height_max; > > > + > > > + /* > > > + * intel_mode_valid() should be > > > + * sufficient on older platforms. > > > + */ > > > + if (INTEL_GEN(dev_priv) < 9) > > > + return MODE_OK; > > > + > > > + /* > > > + * Most people will probably want a fullscreen > > > + * plane so let's not advertize modes that are > > > + * too big for that. > > > + */ > > > + if (INTEL_GEN(dev_priv) >= 11) { > > > + plane_width_max = 5120; > > > + plane_height_max = 4320; > > > + } else { > > > + plane_width_max = 5120; > > > + plane_height_max = 4096; > > > + } > > > + > > > + if (mode->hdisplay > plane_width_max) > > > + return MODE_H_ILLEGAL; > > > + > > > + if (mode->vdisplay > plane_height_max) > > > + return MODE_V_ILLEGAL; > > > + > > > + return MODE_OK; > > > +} > > > + > > > static const struct drm_mode_config_funcs intel_mode_funcs = { > > > .fb_create = intel_user_framebuffer_create, > > > .get_format_info = intel_get_format_info, > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > > > index 66330fcb10d4..b1ae0e59c715 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > > @@ -32,6 +32,7 @@ enum link_m_n_set; > > > struct dpll; > > > struct drm_connector; > > > struct drm_device; > > > +struct drm_display_mode; > > > struct drm_encoder; > > > struct drm_file; > > > struct drm_format_info; > > > @@ -448,6 +449,9 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); > > > u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, > > > u32 pixel_format, u64 modifier); > > > bool intel_plane_can_remap(const struct intel_plane_state *plane_state); > > > +enum drm_mode_status > > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > > + const struct drm_display_mode *mode); > > > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port); > > > > > > void intel_plane_destroy(struct drm_plane *plane); > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index d09133a958e1..ccaf9f00b747 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -562,7 +562,7 @@ intel_dp_mode_valid(struct drm_connector *connector, > > > if (mode->flags & DRM_MODE_FLAG_DBLCLK) > > > return MODE_H_ILLEGAL; > > > > > > - return MODE_OK; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > u32 intel_dp_pack_aux(const u8 *src, int src_bytes) > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c > > > index 5fec02aceaed..a2a937109a5a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dsi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dsi.c > > > @@ -55,6 +55,7 @@ int intel_dsi_get_modes(struct drm_connector *connector) > > > enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_connector *intel_connector = to_intel_connector(connector); > > > const struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode; > > > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > > > @@ -73,7 +74,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > > return MODE_CLOCK_HIGH; > > > } > > > > > > - return MODE_OK; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > struct intel_dsi_host *intel_dsi_host_init(struct intel_dsi *intel_dsi, > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > index c500fc9154c8..dbc686515dce 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > @@ -2188,8 +2188,10 @@ intel_hdmi_mode_valid(struct drm_connector *connector, > > > status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, > > > true, force_dvi); > > > } > > > + if (status != MODE_OK) > > > + return status; > > > > > > - return status; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > > -- > > > 2.21.0 > > > > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > -- > Ville Syrjälä > Intel
On Wed, Sep 18, 2019 at 07:02:18PM +0300, Ville Syrjälä wrote: > On Wed, Sep 18, 2019 at 11:24:09AM -0400, Sean Paul wrote: > > On Wed, Sep 18, 2019 at 06:07:07PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > > > planes' max resolution. This has the nasty implication that modes on the > > > connectors' mode list may not be usable when the user asks for a > > > fullscreen plane. Seeing as that is the most common use case it seems > > > prudent to filter out modes that don't allow for fullscreen planes to > > > be enabled. > > > > > > Let's do that in the connetor .mode_valid() hook so that normally > > > such modes are kept hidden but the user is still able to forcibly > > > specify such a mode if they know they don't need fullscreen planes. > > > > > > This is in line with ealier policies regarding certain clock limits. > > > The idea is to prevent the casual user from encountering a mode that > > > would fail under typical conditions, but allow the expert user to > > > force things if they so wish. > > > > Isn't this exactly what atomic_check is for? Why not just add a debug message in > > get_max_plane_size to leave a breadcrumb? > > There's already a debug message. Won't really help when the screen fails > to light up automagically on account of the preferred mode being too > big. That's not the kernel's fault, why are we working around it at this level? There are lots of reasons beyond max plane size that can cause a modeset to fail. If userspace doesn't already have the smarts to fallback to a lower resolution on modeset failure, we should fix it or just ¯\_(ツ)_/¯ Sean > > > > > Sean > > > > > > > > Maybe in the future we should consider automagically using two > > > planes when one can't cover the entire screen? Wouldn't be a > > > great match for the current uapi with explicit planes though, > > > but I guess no worse than using two pipes (which we apparently > > > have to in the future anyway). Either that or we'd have to > > > teach userspace to do it for us. > > > > > > v2: Fix icl+ max plane heigth (Manasi) > > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > > Cc: Leho Kraav <leho@kraav.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_display.h | 4 +++ > > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_dsi.c | 3 +- > > > drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++- > > > 5 files changed, 46 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index f5e65e052d91..0147e4ae3622 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -15753,6 +15753,7 @@ intel_mode_valid(struct drm_device *dev, > > > DRM_MODE_FLAG_CLKDIV2)) > > > return MODE_BAD; > > > > > > + /* Transcoder timing limits */ > > > if (INTEL_GEN(dev_priv) >= 11) { > > > hdisplay_max = 16384; > > > vdisplay_max = 8192; > > > @@ -15791,6 +15792,41 @@ intel_mode_valid(struct drm_device *dev, > > > return MODE_OK; > > > } > > > > > > +enum drm_mode_status > > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > > + const struct drm_display_mode *mode) > > > +{ > > > + int plane_width_max, plane_height_max; > > > + > > > + /* > > > + * intel_mode_valid() should be > > > + * sufficient on older platforms. > > > + */ > > > + if (INTEL_GEN(dev_priv) < 9) > > > + return MODE_OK; > > > + > > > + /* > > > + * Most people will probably want a fullscreen > > > + * plane so let's not advertize modes that are > > > + * too big for that. > > > + */ > > > + if (INTEL_GEN(dev_priv) >= 11) { > > > + plane_width_max = 5120; > > > + plane_height_max = 4320; > > > + } else { > > > + plane_width_max = 5120; > > > + plane_height_max = 4096; > > > + } > > > + > > > + if (mode->hdisplay > plane_width_max) > > > + return MODE_H_ILLEGAL; > > > + > > > + if (mode->vdisplay > plane_height_max) > > > + return MODE_V_ILLEGAL; > > > + > > > + return MODE_OK; > > > +} > > > + > > > static const struct drm_mode_config_funcs intel_mode_funcs = { > > > .fb_create = intel_user_framebuffer_create, > > > .get_format_info = intel_get_format_info, > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > > > index 66330fcb10d4..b1ae0e59c715 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > > @@ -32,6 +32,7 @@ enum link_m_n_set; > > > struct dpll; > > > struct drm_connector; > > > struct drm_device; > > > +struct drm_display_mode; > > > struct drm_encoder; > > > struct drm_file; > > > struct drm_format_info; > > > @@ -448,6 +449,9 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); > > > u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, > > > u32 pixel_format, u64 modifier); > > > bool intel_plane_can_remap(const struct intel_plane_state *plane_state); > > > +enum drm_mode_status > > > +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, > > > + const struct drm_display_mode *mode); > > > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port); > > > > > > void intel_plane_destroy(struct drm_plane *plane); > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index d09133a958e1..ccaf9f00b747 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -562,7 +562,7 @@ intel_dp_mode_valid(struct drm_connector *connector, > > > if (mode->flags & DRM_MODE_FLAG_DBLCLK) > > > return MODE_H_ILLEGAL; > > > > > > - return MODE_OK; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > u32 intel_dp_pack_aux(const u8 *src, int src_bytes) > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c > > > index 5fec02aceaed..a2a937109a5a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dsi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dsi.c > > > @@ -55,6 +55,7 @@ int intel_dsi_get_modes(struct drm_connector *connector) > > > enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_connector *intel_connector = to_intel_connector(connector); > > > const struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode; > > > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > > > @@ -73,7 +74,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, > > > return MODE_CLOCK_HIGH; > > > } > > > > > > - return MODE_OK; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > struct intel_dsi_host *intel_dsi_host_init(struct intel_dsi *intel_dsi, > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > index c500fc9154c8..dbc686515dce 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > @@ -2188,8 +2188,10 @@ intel_hdmi_mode_valid(struct drm_connector *connector, > > > status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, > > > true, force_dvi); > > > } > > > + if (status != MODE_OK) > > > + return status; > > > > > > - return status; > > > + return intel_mode_valid_max_plane_size(dev_priv, mode); > > > } > > > > > > static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > > -- > > > 2.21.0 > > > > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > -- > Ville Syrjälä > Intel
Op 18-09-2019 om 17:07 schreef Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > planes' max resolution. This has the nasty implication that modes on the > connectors' mode list may not be usable when the user asks for a > fullscreen plane. Seeing as that is the most common use case it seems > prudent to filter out modes that don't allow for fullscreen planes to > be enabled. > > Let's do that in the connetor .mode_valid() hook so that normally > such modes are kept hidden but the user is still able to forcibly > specify such a mode if they know they don't need fullscreen planes. > > This is in line with ealier policies regarding certain clock limits. > The idea is to prevent the casual user from encountering a mode that > would fail under typical conditions, but allow the expert user to > force things if they so wish. > > Maybe in the future we should consider automagically using two > planes when one can't cover the entire screen? Wouldn't be a > great match for the current uapi with explicit planes though, > but I guess no worse than using two pipes (which we apparently > have to in the future anyway). Either that or we'd have to > teach userspace to do it for us. In theory this is what bigjoiner is doing, except the planes are on a different pipe. Will be fun with SDR vs HDR planes though..
On Wed, Sep 18, 2019 at 04:21:06PM -0400, Sean Paul wrote: > On Wed, Sep 18, 2019 at 07:02:18PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 18, 2019 at 11:24:09AM -0400, Sean Paul wrote: > > > On Wed, Sep 18, 2019 at 06:07:07PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Modern platforms allow the transcoders hdisplay/vdisplay to exceed the > > > > planes' max resolution. This has the nasty implication that modes on the > > > > connectors' mode list may not be usable when the user asks for a > > > > fullscreen plane. Seeing as that is the most common use case it seems > > > > prudent to filter out modes that don't allow for fullscreen planes to > > > > be enabled. > > > > > > > > Let's do that in the connetor .mode_valid() hook so that normally > > > > such modes are kept hidden but the user is still able to forcibly > > > > specify such a mode if they know they don't need fullscreen planes. > > > > > > > > This is in line with ealier policies regarding certain clock limits. > > > > The idea is to prevent the casual user from encountering a mode that > > > > would fail under typical conditions, but allow the expert user to > > > > force things if they so wish. > > > > > > Isn't this exactly what atomic_check is for? Why not just add a debug message in > > > get_max_plane_size to leave a breadcrumb? > > > > There's already a debug message. Won't really help when the screen fails > > to light up automagically on account of the preferred mode being too > > big. > > That's not the kernel's fault, why are we working around it at this level? There > are lots of reasons beyond max plane size that can cause a modeset to fail. If > userspace doesn't already have the smarts to fallback to a lower resolution on > modeset failure, we should fix it or just ¯\_(ツ)_/¯ Sure, userspace (and fb_helper) should be smarter about this. Unfortunately I don't have a time machine to deploy such a backport so this is the best I can do for current userspace.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index f5e65e052d91..0147e4ae3622 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -15753,6 +15753,7 @@ intel_mode_valid(struct drm_device *dev, DRM_MODE_FLAG_CLKDIV2)) return MODE_BAD; + /* Transcoder timing limits */ if (INTEL_GEN(dev_priv) >= 11) { hdisplay_max = 16384; vdisplay_max = 8192; @@ -15791,6 +15792,41 @@ intel_mode_valid(struct drm_device *dev, return MODE_OK; } +enum drm_mode_status +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, + const struct drm_display_mode *mode) +{ + int plane_width_max, plane_height_max; + + /* + * intel_mode_valid() should be + * sufficient on older platforms. + */ + if (INTEL_GEN(dev_priv) < 9) + return MODE_OK; + + /* + * Most people will probably want a fullscreen + * plane so let's not advertize modes that are + * too big for that. + */ + if (INTEL_GEN(dev_priv) >= 11) { + plane_width_max = 5120; + plane_height_max = 4320; + } else { + plane_width_max = 5120; + plane_height_max = 4096; + } + + if (mode->hdisplay > plane_width_max) + return MODE_H_ILLEGAL; + + if (mode->vdisplay > plane_height_max) + return MODE_V_ILLEGAL; + + return MODE_OK; +} + static const struct drm_mode_config_funcs intel_mode_funcs = { .fb_create = intel_user_framebuffer_create, .get_format_info = intel_get_format_info, diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index 66330fcb10d4..b1ae0e59c715 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -32,6 +32,7 @@ enum link_m_n_set; struct dpll; struct drm_connector; struct drm_device; +struct drm_display_mode; struct drm_encoder; struct drm_file; struct drm_format_info; @@ -448,6 +449,9 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, u32 pixel_format, u64 modifier); bool intel_plane_can_remap(const struct intel_plane_state *plane_state); +enum drm_mode_status +intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv, + const struct drm_display_mode *mode); enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port); void intel_plane_destroy(struct drm_plane *plane); diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index d09133a958e1..ccaf9f00b747 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -562,7 +562,7 @@ intel_dp_mode_valid(struct drm_connector *connector, if (mode->flags & DRM_MODE_FLAG_DBLCLK) return MODE_H_ILLEGAL; - return MODE_OK; + return intel_mode_valid_max_plane_size(dev_priv, mode); } u32 intel_dp_pack_aux(const u8 *src, int src_bytes) diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c index 5fec02aceaed..a2a937109a5a 100644 --- a/drivers/gpu/drm/i915/display/intel_dsi.c +++ b/drivers/gpu/drm/i915/display/intel_dsi.c @@ -55,6 +55,7 @@ int intel_dsi_get_modes(struct drm_connector *connector) enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { + struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_connector *intel_connector = to_intel_connector(connector); const struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode; int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; @@ -73,7 +74,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector, return MODE_CLOCK_HIGH; } - return MODE_OK; + return intel_mode_valid_max_plane_size(dev_priv, mode); } struct intel_dsi_host *intel_dsi_host_init(struct intel_dsi *intel_dsi, diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index c500fc9154c8..dbc686515dce 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2188,8 +2188,10 @@ intel_hdmi_mode_valid(struct drm_connector *connector, status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, true, force_dvi); } + if (status != MODE_OK) + return status; - return status; + return intel_mode_valid_max_plane_size(dev_priv, mode); } static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state,