Re: [PATCH] drm: move check for min/max width/height for atomic drivers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 03, 2016 at 09:35:24AM -0600, Sean Paul wrote:
> On Thu, Nov 3, 2016 at 9:22 AM, Rob Clark <robdclark@gmail.com> wrote:
> > On Thu, Nov 3, 2016 at 10:55 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Thu, Nov 03, 2016 at 10:14:20AM -0400, Rob Clark wrote:
> >>> On Thu, Nov 3, 2016 at 10:12 AM, Ville Syrjälä
> >>> <ville.syrjala@linux.intel.com> wrote:
> >>> > On Wed, Nov 02, 2016 at 10:27:44AM -0400, Rob Clark wrote:
> >>> >> drm-hwc + android tries to create an fb for the wallpaper layer, which
> >>> >> is larger than the screen resolution, and potentially larger than
> >>> >> mode_config->max_{width,height}.  But the plane src_w/src_h is within
> >>> >> the max limits, so it is something the hardware can otherwise do.
> >>> >>
> >>> >> For atomic drivers, defer the check to drm_atomic_plane_check().
> >>> >>
> >>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >>> >> ---
> >>> >>  drivers/gpu/drm/drm_atomic.c | 17 +++++++++++++++++
> >>> >>  drivers/gpu/drm/drm_crtc.c   | 26 +++++++++++++++++---------
> >>> >>  2 files changed, 34 insertions(+), 9 deletions(-)
> >>> >>
> >>> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> >> index 34edd4f..fb0f07ce 100644
> >>> >> --- a/drivers/gpu/drm/drm_atomic.c
> >>> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> >> @@ -846,7 +846,9 @@ plane_switching_crtc(struct drm_atomic_state *state,
> >>> >>  static int drm_atomic_plane_check(struct drm_plane *plane,
> >>> >>               struct drm_plane_state *state)
> >>> >>  {
> >>> >> +     struct drm_mode_config *config = &plane->dev->mode_config;
> >>> >>       unsigned int fb_width, fb_height;
> >>> >> +     unsigned int min_width, max_width, min_height, max_height;
> >>> >>       int ret;
> >>> >>
> >>> >>       /* either *both* CRTC and FB must be set, or neither */
> >>> >> @@ -909,6 +911,21 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >>> >>               return -ENOSPC;
> >>> >>       }
> >>> >>
> >>> >> +     min_width = config->min_width << 16;
> >>> >> +     max_width = config->max_width << 16;
> >>> >> +     min_height = config->min_height << 16;
> >>> >> +     max_height = config->max_height << 16;
> >>> >> +
> >>> >> +     /* Make sure source dimensions are within bounds. */
> >>> >> +     if (min_width > state->src_w || state->src_w > max_width ||
> >>> >> +         min_height > state->src_h || state->src_h > max_height) {
> >>> >> +             DRM_DEBUG_ATOMIC("Invalid source size "
> >>> >> +                              "%u.%06ux%u.%06u\n",
> >>> >> +                              state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> >>> >> +                              state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
> >>> >> +             return -ERANGE;
> >>> >> +     }
> >>> >> +
> >>> >>       if (plane_switching_crtc(state->state, plane, state)) {
> >>> >>               DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
> >>> >>                                plane->base.id, plane->name);
> >>> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>> >> index b4b973f..7294bde 100644
> >>> >> --- a/drivers/gpu/drm/drm_crtc.c
> >>> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >>> >> @@ -3369,15 +3369,23 @@ internal_framebuffer_create(struct drm_device *dev,
> >>> >>               return ERR_PTR(-EINVAL);
> >>> >>       }
> >>> >>
> >>> >> -     if ((config->min_width > r->width) || (r->width > config->max_width)) {
> >>> >> -             DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
> >>> >> -                       r->width, config->min_width, config->max_width);
> >>> >> -             return ERR_PTR(-EINVAL);
> >>> >> -     }
> >>> >> -     if ((config->min_height > r->height) || (r->height > config->max_height)) {
> >>> >> -             DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
> >>> >> -                       r->height, config->min_height, config->max_height);
> >>> >> -             return ERR_PTR(-EINVAL);
> >>> >> +     /* for atomic drivers, we check the src dimensions in
> >>> >> +      * drm_atomic_plane_check().. allow creation of a fb
> >>> >> +      * that is larger than what can be scanned out, as
> >>> >> +      * long as userspace doesn't try to scanout a portion
> >>> >> +      * of the fb that is too large.
> >>> >> +      */
> >>> >> +     if (!file_priv->atomic) {
> >>> >> +             if ((config->min_width > r->width) || (r->width > config->max_width)) {
> >>> >> +                     DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
> >>> >> +                               r->width, config->min_width, config->max_width);
> >>> >> +                     return ERR_PTR(-EINVAL);
> >>> >> +             }
> >>> >> +             if ((config->min_height > r->height) || (r->height > config->max_height)) {
> >>> >> +                     DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
> >>> >> +                               r->height, config->min_height, config->max_height);
> >>> >> +                     return ERR_PTR(-EINVAL);
> >>> >> +             }
> >>> >
> >>> > So why not just bump max_foo in your driver?
> >>> >
> >>> > Removing the restriction from the core seems likely to break some
> >>> > drivers as they would now have to check the fb dimensions themselves.
> >>>
> >>> that is why I did it only for atomic drivers, so we could rely on the
> >>> checking in drm_atomic_plane_check()..
> >>
> >> That's not used to check the framebuffer dimensions.
> >
> > but it is used to check scanout dimensions and that is usually what
> > matters..  we could add a max-pitch param if needed, but the max-fb
> > dimension check is mostly useless.
> >
> 
> Yeah, I suppose this depends on how you interpret the mode_config
> constraints. I think this change makes sense since we shouldn't limit
> fb size on scanout restrictions.

Hmm. So what are the uses we have for this information?

First one is the max fb size, the other one is mode filtering.
So I'm thinking we could just split the fb size limit into its
own thing and leave the current thing indicating the limits of
the timing generators hdisplay/vdisplay for mode filtering.

Although drivers should still filter the modes further based on
the other timings as well, so not sure how much use there is
in having the core do part of it. But I think currently many
drivers might not do the filtering very robustly so having
something in the core is probably better than nothing.

Additionally planes could have additional scanout limitations,
but those should already be handled by any reasonably competemnt
atomic_check implementation. Although I suspect a bunch of things
would go a bit nuts if we couldn't support an unscaled full
screen primary plane. So potentially the mode_config limits
should be the minimum of the timing generator and plane scanout
limits? I suppose the two would ususally be matched in the hardware
anyway.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel