diff mbox

drm: Document mode_config.max_width/height as the max fb dimensions

Message ID 20180615173939.11353-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä June 15, 2018, 5:39 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The meaning of the mode_config max_width/height fields has not been
entirely clear. They are used both as the max framebuffer dimensions,
and they are also used by drm_mode_getconnector() to filter out
any mode whose hdisplay/vdisplay exceed those limits.

Let's put it in writing that max_width/height only refrer to the max
framebuffer dimensions, and should those be higher than the hardware
limits for display timings the driver must validate the latter using
some other means.

We'll keep the max_width/height usage in drm_mode_getconnector()
because setcrtc treats hdisplay/vdisplay also as the primary plane
width, and having a plane bigger than the max fb size doesn't make
much sense (if we ignore scaling that is). It all works out fine
as long as the max fb dimensions are at least equal to the max
timing limits. If the opposite were true we may want to rethink
what drm_mode_getconnector() does. Maybe do the mode filtering
only for non-atomic userspace?

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_mode_config.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Navare, Manasi June 15, 2018, 7:34 p.m. UTC | #1 
On Fri, Jun 15, 2018 at 08:39:39PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The meaning of the mode_config max_width/height fields has not been
> entirely clear. They are used both as the max framebuffer dimensions,
> and they are also used by drm_mode_getconnector() to filter out
> any mode whose hdisplay/vdisplay exceed those limits.
> 
> Let's put it in writing that max_width/height only refrer to the max
> framebuffer dimensions, and should those be higher than the hardware
> limits for display timings the driver must validate the latter using
> some other means.
> 
> We'll keep the max_width/height usage in drm_mode_getconnector()
> because setcrtc treats hdisplay/vdisplay also as the primary plane
> width, and having a plane bigger than the max fb size doesn't make
> much sense (if we ignore scaling that is). It all works out fine
> as long as the max fb dimensions are at least equal to the max
> timing limits. If the opposite were true we may want to rethink
> what drm_mode_getconnector() does. Maybe do the mode filtering
> only for non-atomic userspace?
>

Yes this makes sense and as we support higher resolutions, we need to update
the fb_width and fb_height maximum values so those modes dont get
filtered out.

This looks good to me.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi
 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  include/drm/drm_mode_config.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index fb45839179dd..f796691b2d09 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -329,10 +329,10 @@ struct drm_mode_config_funcs {
>  
>  /**
>   * struct drm_mode_config - Mode configuration control structure
> - * @min_width: minimum pixel width on this device
> - * @min_height: minimum pixel height on this device
> - * @max_width: maximum pixel width on this device
> - * @max_height: maximum pixel height on this device
> + * @min_width: minimum fb pixel width on this device
> + * @min_height: minimum fb pixel height on this device
> + * @max_width: maximum fb pixel width on this device
> + * @max_height: maximum fb pixel height on this device
>   * @funcs: core driver provided mode setting functions
>   * @fb_base: base address of the framebuffer
>   * @poll_enabled: track polling support for this device
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter June 18, 2018, 9:06 a.m. UTC | #2 
On Fri, Jun 15, 2018 at 08:39:39PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The meaning of the mode_config max_width/height fields has not been
> entirely clear. They are used both as the max framebuffer dimensions,
> and they are also used by drm_mode_getconnector() to filter out
> any mode whose hdisplay/vdisplay exceed those limits.
> 
> Let's put it in writing that max_width/height only refrer to the max
> framebuffer dimensions, and should those be higher than the hardware
> limits for display timings the driver must validate the latter using
> some other means.
> 
> We'll keep the max_width/height usage in drm_mode_getconnector()
> because setcrtc treats hdisplay/vdisplay also as the primary plane
> width, and having a plane bigger than the max fb size doesn't make
> much sense (if we ignore scaling that is). It all works out fine
> as long as the max fb dimensions are at least equal to the max
> timing limits. If the opposite were true we may want to rethink
> what drm_mode_getconnector() does. Maybe do the mode filtering
> only for non-atomic userspace?

Defacto uapi is that if you do a modeset using the primary plane at full
res using a XRGB8888 format, it Will Work (tm). There's way too much
userspace out there that just blindly assumes that, and I think drivers
need to cope with that expectation.

That's why we have format conversion code in tinydrm, and that's why we
have fancy code in msm to use multiple hw planes if the framebuffer is
over the limit. So yeah, I think documentating the expectation that fb
limits >= scanout limits as a hard requirement for reasonable drivers
(i.e. stuff you expect a normal linux distro to boot on) seems reasonable.

Just as an aside, if you want to clarify this furhter. Ack on the patch
itself.

Cheers, Daniel
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  include/drm/drm_mode_config.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index fb45839179dd..f796691b2d09 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -329,10 +329,10 @@ struct drm_mode_config_funcs {
>  
>  /**
>   * struct drm_mode_config - Mode configuration control structure
> - * @min_width: minimum pixel width on this device
> - * @min_height: minimum pixel height on this device
> - * @max_width: maximum pixel width on this device
> - * @max_height: maximum pixel height on this device
> + * @min_width: minimum fb pixel width on this device
> + * @min_height: minimum fb pixel height on this device
> + * @max_width: maximum fb pixel width on this device
> + * @max_height: maximum fb pixel height on this device
>   * @funcs: core driver provided mode setting functions
>   * @fb_base: base address of the framebuffer
>   * @poll_enabled: track polling support for this device
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index fb45839179dd..f796691b2d09 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -329,10 +329,10 @@  struct drm_mode_config_funcs {
 
 /**
  * struct drm_mode_config - Mode configuration control structure
- * @min_width: minimum pixel width on this device
- * @min_height: minimum pixel height on this device
- * @max_width: maximum pixel width on this device
- * @max_height: maximum pixel height on this device
+ * @min_width: minimum fb pixel width on this device
+ * @min_height: minimum fb pixel height on this device
+ * @max_width: maximum fb pixel width on this device
+ * @max_height: maximum fb pixel height on this device
  * @funcs: core driver provided mode setting functions
  * @fb_base: base address of the framebuffer
  * @poll_enabled: track polling support for this device