All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Paul <seanpaul@chromium.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 04/10] drm: Extract drm_plane.[hc]
Date: Wed, 21 Sep 2016 10:32:26 +0300	[thread overview]
Message-ID: <CAEqLBRkpqGEGinvCq1avmOWO8NQTLH2oAoZf4i-xKC44R4yGxw@mail.gmail.com> (raw)
In-Reply-To: <20160919131150.GI20761@phenom.ffwll.local>

On Mon, Sep 19, 2016 at 4:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.

That's fair.

>>
>>
>> <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?

Yeah, I see what you're getting at. I'm still on the fence, but it'll
be slightly awkward in either place.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

Sean

> -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-09-21  7:32 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-31 16:09 [PATCH 00/10] More splitting&documenting for drm_crtc.c Daniel Vetter
2016-08-31 16:09 ` [PATCH 01/10] drm: Move a few macros away from drm_crtc.h Daniel Vetter
2016-09-06 16:59   ` [Intel-gfx] " Sean Paul
2016-09-08  0:05   ` Carlos Santa
2016-08-31 16:09 ` [PATCH 02/10] drm: Extract drm_bridge.h Daniel Vetter
2016-09-02  9:25   ` Archit Taneja
2016-08-31 16:09 ` [PATCH 03/10] drm: Move all decl for drm_edid.c to drm_edid.h Daniel Vetter
2016-09-06 16:59   ` Sean Paul
2016-09-19 14:28     ` Daniel Vetter
2016-08-31 16:09 ` [PATCH 04/10] drm: Extract drm_plane.[hc] Daniel Vetter
2016-09-06 16:59   ` [Intel-gfx] " Sean Paul
2016-09-19 13:11     ` Daniel Vetter
2016-09-21  7:32       ` Sean Paul [this message]
2016-08-31 16:09 ` [PATCH 05/10] drm/doc: Polish for drm_plane.[hc] Daniel Vetter
2016-09-02  9:30   ` Archit Taneja
2016-09-19 13:13     ` Daniel Vetter
2016-09-21  6:38       ` Archit Taneja
2016-08-31 16:09 ` [PATCH 06/10] drm: Conslidate blending properties in drm_blend.[hc] Daniel Vetter
2016-09-06 16:59   ` Sean Paul
2016-08-31 16:09 ` [PATCH 07/10] drm/doc: Polish plane composition property docs Daniel Vetter
2016-09-06 16:59   ` Sean Paul
2016-08-31 16:09 ` [PATCH 08/10] drm: Extract drm_color_mgmt.[hc] Daniel Vetter
2016-09-01  9:51   ` [Intel-gfx] " Lionel Landwerlin
2016-08-31 16:09 ` [PATCH 09/10] drm/doc: Document color space handling Daniel Vetter
2016-09-01 10:16   ` Lionel Landwerlin
2016-09-06 16:59   ` [Intel-gfx] " Sean Paul
2016-08-31 16:09 ` [PATCH 10/10] drm: Remove dirty property from docs Daniel Vetter
2016-09-06 16:59   ` Sean Paul
2016-09-01  9:20 ` ✗ Fi.CI.BAT: failure for More splitting&documenting for drm_crtc.c Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEqLBRkpqGEGinvCq1avmOWO8NQTLH2oAoZf4i-xKC44R4yGxw@mail.gmail.com \
    --to=seanpaul@chromium.org \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --subject='Re: [PATCH 04/10] drm: Extract drm_plane.[hc]' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.