On Thu, Nov 3, 2016 at 9:22 AM, Rob Clark <robdclark@xxxxxxxxx> 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.
>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
