Re: [PATCH 04/10] drm: Extract drm_plane.[hc]

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 06, 2016 at 12:59:31PM -0400, Sean Paul wrote:
> On Wed, Aug 31, 2016 at 12:09 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Just pure code movement, cleanup and polish will happen in later
> > patches.
> >
> > v2: Don't forget all the ioctl! To extract those cleanly I decided to
> > put check_src_coords into drm_framebuffer.c (and give it a
> > drm_framebuffer_ prefix), since that just checks framebuffer
> > constraints.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/gpu/drm-kms.rst       |  12 +
> >  drivers/gpu/drm/Makefile            |   3 +-
> >  drivers/gpu/drm/drm_crtc.c          | 939 +-----------------------------------
> >  drivers/gpu/drm/drm_crtc_internal.h |  38 +-
> >  drivers/gpu/drm/drm_framebuffer.c   |  26 +
> >  drivers/gpu/drm/drm_plane.c         | 937 +++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic.h            | 154 ++++++
> >  include/drm/drm_crtc.h              | 583 +---------------------
> >  include/drm/drm_plane.h             | 470 ++++++++++++++++++
> >  9 files changed, 1628 insertions(+), 1534 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_plane.c
> >  create mode 100644 include/drm/drm_plane.h
> >
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index f9a991bb87d4..33181be97151 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -110,6 +110,18 @@ Note that dumb objects may not be used for gpu acceleration, as has been
> >  attempted on some ARM embedded platforms. Such drivers really must have
> >  a hardware-specific ioctl to allocate suitable buffer objects.
> >
> > +Plane Abstraction
> > +=================
> > +
> > +Plane Functions Reference
> > +-------------------------
> > +
> > +.. kernel-doc:: include/drm/drm_plane.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
> > +   :export:
> > +
> >  Display Modes Function Reference
> >  ================================
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 439d89b25ae0..8eeb07a35798 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -14,7 +14,8 @@ drm-y       :=        drm_auth.o drm_bufs.o drm_cache.o \
> >                 drm_rect.o drm_vma_manager.o drm_flip_work.o \
> >                 drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> >                 drm_framebuffer.o drm_connector.o drm_blend.o \
> > -               drm_encoder.o drm_mode_object.o drm_property.o
> > +               drm_encoder.o drm_mode_object.o drm_property.o \
> > +               drm_plane.o
> >
> >  drm-$(CONFIG_COMPAT) += drm_ioc32.o
> >  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 0fad433f4d2d..513ab4729683 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> 
> <snip>
> 
> > -
> > -static int check_src_coords(uint32_t src_x, uint32_t src_y,
> > -                           uint32_t src_w, uint32_t src_h,
> > -                           const struct drm_framebuffer *fb)
> > -{
> > -       unsigned int fb_width, fb_height;
> > -
> > -       fb_width = fb->width << 16;
> > -       fb_height = fb->height << 16;
> > -
> > -       /* Make sure source coordinates are inside the fb. */
> > -       if (src_w > fb_width ||
> > -           src_x > fb_width - src_w ||
> > -           src_h > fb_height ||
> > -           src_y > fb_height - src_h) {
> > -               DRM_DEBUG_KMS("Invalid source coordinates "
> > -                             "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> > -                             src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
> > -                             src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
> > -                             src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
> > -                             src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
> > -               return -ENOSPC;
> > -       }
> > -
> > -       return 0;
> > -}
> 
> I'm good with this change, but I'd argue that it probably belongs in
> its own patch.

Except for moving the function + giving it a prefix (since it's no longer
static) there's no change here.
> 
> 
> <snip>
> 
> >  /**
> > - * drm_mode_page_flip_ioctl - schedule an asynchronous fb update
> > - * @dev: DRM device
> > - * @data: ioctl data
> > - * @file_priv: DRM file info
> > - *
> > - * This schedules an asynchronous update on a given CRTC, called page flip.
> > - * Optionally a drm event is generated to signal the completion of the event.
> > - * Generic drivers cannot assume that a pageflip with changed framebuffer
> > - * properties (including driver specific metadata like tiling layout) will work,
> > - * but some drivers support e.g. pixel format changes through the pageflip
> > - * ioctl.
> > - *
> > - * Called by the user via ioctl.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > -                            void *data, struct drm_file *file_priv)
> 
> 
> IMO, this makes more sense where it is (it's a crtc operation since
> the ioctl data doesn't even reference planes). Perhaps it should be
> sent out on an icefloat with setplane and other legacy ABI in some
> corner.

Ever since universal planes that's conceptually no longer true - we flip
the primary plane, not the entire CRTC. That the ioctl struct takes the
CRTC id is just a historical artifacat of our evolved interface. That's
why I think the page flip ioctl belongs into drm_plane.c, and for the same
reasons I've also moved the legacy cursor ioctls. And yes it's somewhat
inconsistent that set_config will stay in drm_crtc.c, since that both
updates the CRTC's mode, and the primary plane's fb. But given that the
implementation of the same in drm_crtc_helper.c is 90% concerned with
doing modesets it makes sense to keep it near the CRTC code.

And once those ioctls are there it imo also makes sense to move
check_src_coords.

Convinced?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx