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

[Rob Clark]

On Thu, Nov 3, 2016 at 2:32 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Thu, Nov 03, 2016 at 11:22:37AM -0400, Rob Clark wrote:
>> On Thu, Nov 3, 2016 at 10:55 AM, Ville Syrjälä
>> <ville.syrjala at 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 at 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 at 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.
>
> There's no point in letting the user create a framebuffer that can't be
> scanned out at all.

But if a user can scan out *part* of it.. which is the case here.  And
a common case.

tbh, our min/max checks are pretty bogus for most hardware, other than
maybe tilcdc where width==pitch...  it should be more along the lines
of max_pitch or stride, plus src_w/src_h checks in
drm_atomic_plane_check().

BR,
-R