On Thu, Nov 03, 2016 at 02:37:30PM -0400, Rob Clark wrote:
> On Thu, Nov 3, 2016 at 2:32 PM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >> >> >> ---
> >> >> >> 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.
That's not how I interpret these limits. In my book they're supposed to
be the max fb size you can scan out (or what you're willing to support
in your driver). Whether you can scan out just a part of it is another
matter (and a matter for atomic_check() at that).
>
> 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().
Yeah, I guess stride is often the limiting factor. Although with
rotation even the vertical stride might start to matter.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
