| Message ID | 1478096864-14639-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 2, 2016 at 10:27 AM, Rob Clark <robdclark@gmail.com> 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) { btw, possibly we should loosen this restriction to just DRIVER_ATOMIC? It is a problem w/ x11 and multiple screens as well. BR, -R > + 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); > + } > } > > if (r->flags & DRM_MODE_FB_MODIFIERS && > -- > 2.7.4 >
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. > } > > if (r->flags & DRM_MODE_FB_MODIFIERS && > -- > 2.7.4
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().. BR, -R >> } >> >> if (r->flags & DRM_MODE_FB_MODIFIERS && >> -- >> 2.7.4 > > -- > Ville Syrjälä > Intel OTC
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.
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. BR, -R > -- > Ville Syrjälä > Intel OTC
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. Sean > BR, > -R > >> -- >> Ville Syrjälä >> Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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. There's no point in letting the user create a framebuffer that can't be scanned out at all.
On Thu, Nov 3, 2016 at 2:32 PM, Ville Syrjälä <ville.syrjala@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@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. > > 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
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.
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@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@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. > > > > 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.
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> Top-posting since the discussion kinda disintegrated, but I think an optional max_fb_w/h (if 0 we use the screen limits) would be the better approach. More common code ftw at least. -Daniel > --- > 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); > + } > } > > if (r->flags & DRM_MODE_FB_MODIFIERS && > -- > 2.7.4 >
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); + } } if (r->flags & DRM_MODE_FB_MODIFIERS &&
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(-)