[PATCH] drm/exynos: wait for the completion of pending page flip
Inki Dae
inki.dae at samsung.com
Wed May 22 01:59:00 PDT 2013
2013/5/22 Kyungmin Park <kmpark at infradead.org>
>
>
> On Wed, May 22, 2013 at 5:23 PM, Tomasz Figa <tomasz.figa at gmail.com>wrote:
>
>> Hi Inki,
>>
>> On Wednesday 22 of May 2013 15:37:14 Inki Dae wrote:
>> > 2013/5/22 Stéphane Marchesin <stephane.marchesin at gmail.com>
>> >
>> > > On Tue, May 21, 2013 at 9:22 PM, Inki Dae <inki.dae at samsung.com>
>> wrote:
>> > > > 2013/5/22 Stéphane Marchesin <stephane.marchesin at gmail.com>
>> > > >
>> > > >> On Tue, May 21, 2013 at 1:08 AM, Inki Dae <inki.dae at samsung.com>
>> wrote:
>> > > >> > This patch fixes the issue that drm_vblank_get() is failed.
>> > > >> >
>> > > >> > The issus occurs when next page flip request is tried
>> > > >> > if previous page flip event wasn't completed yet and then
>> > > >> > dpms became off.
>> > > >> >
>> > > >> > So this patch make sure that page flip event is completed
>> > > >> > before dpms goes to off.
>> > > >>
>> > > >> Hi,
>> > > >>
>> > > >> This patch is a squash of the two following patches from the Chrome
>> > > >> OS
>> > >
>> > > >> tree with the KDS bits removed and the dpms off bit added:
>> > >
>> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.g
>> > > it;a=commitdiff;h=2e77cd4e423967862ca01b1af82aa8b5b7429fc4;hp=aba002da
>> > > 4c6e5efec4d43e1ce33930a79269349a
>> > >
>> > >
>> > >
>> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.g
>> > > it;a=commitdiff;h=b4ec8bfa750ef43a43c2da746c8afdbb51002882;hp=4f28b9a7
>> > > 5c928f229443d7c6c3163159ceb6903a>
>> > > >> Please keep proper attribution.
>> > > >
>> > > > Those patches are just for Chrome OS. Please post them if you want
>> > > > for
>> > >
>> > > those
>> > >
>> > > > to be considered so that they can be reviewed.
>>
> Hi Stephane,
>
>> > >
>> > > We intend to, once they are rebased onto latest kernel. But what I'm
>> > > pointing out is that you're removing proper attribution from Chrome OS
>> > > patches, and this is the second time it has happened.
>>
> First, we don't know what's going on Chrome OS. probably you think we
> refer your codes. but we don't know chrome codes and doesn't refer it. it's
> our finding from our use case.
> Second, samsung has lots of division. some engineers are working with
> Chrome OS. but ours is not their division.
> now we're working on anoter platform and use exynos drm.
> As you know "chinese wall". we're doing it properly.
>
Actually, we referred to i915 codes, intel_display.c :)
>
> Thank you,
> Kyungmin Park
>
>> >
>> > What is mean? does mainline kernel have the attribution? if not so, we
>> > don't need to consider it. And please know that I can not be aware of
>> > you have such patch set without any posting.
>> >
>>
>> at·tri·bu·tion
>> n.
>> 1. The act of attributing, especially the act of establishing a particular
>> person as the creator of a work of art
>> [1]
>>
>> So yes, mainline kernel has attribution. Actually posting something as
>> work of someone that is not the author of the posted work is considered
>> bad everywhere, isn't it?
>>
>> However looking at those two patches linked by Stéphane, I'm not really
>> sure this patch is really just a squash of them. Stéphane, are you 100%
>> sure that your claims are true?
>>
>> Best regards,
>> Tomasz
>>
>> [1] http://www.thefreedictionary.com/attribution
>>
>> > > > That is why we attend open
>> > > > source. One more comment, please do not abuse
>> > > > exynos_drm_crtc_page_flip()>
>> > > What do you mean by abuse?
>> > >
>> > > >> Signed-off-by: Inki Dae <inki.dae at samsung.com>
>> > > >> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> > > >> ---
>> > > >>
>> > > >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 ++++++++++++++++
>> > > >> 1 files changed, 16 insertions(+), 0 deletions(-)
>> > > >>
>> > > >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> > > >> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> > > >> index e8894bc..69a77e9 100644
>> > > >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> > > >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> > > >> @@ -48,6 +48,8 @@ struct exynos_drm_crtc {
>> > > >>
>> > > >> unsigned int pipe;
>> > > >> unsigned int dpms;
>> > > >> enum exynos_crtc_mode mode;
>> > > >>
>> > > >> + wait_queue_head_t pending_flip_queue;
>> > > >> + atomic_t pending_flip;
>> > > >>
>> > > >> };
>> > > >>
>> > > >> static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
>> > > >>
>> > > >> @@ -61,6 +63,13 @@ static void exynos_drm_crtc_dpms(struct drm_crtc
>> > >
>> > > *crtc,
>> > >
>> > > >> int mode)
>> > > >>
>> > > >> return;
>> > > >>
>> > > >> }
>> > > >>
>> > > >> + if (mode > DRM_MODE_DPMS_ON) {
>> > > >> + /* wait for the completion of page flip. */
>> > > >> + wait_event(exynos_crtc->pending_flip_queue,
>> > > >> +
>> > > >> atomic_read(&exynos_crtc->pending_flip)
>> > >
>> > > ==
>> > >
>> > > >> 0);
>> > > >> + drm_vblank_off(crtc->dev, exynos_crtc->pipe);
>> > > >>
>> > > >> You should be using vblank_put/get.
>> > > >
>> > > > No, drm_vblank_put should be called by
>> > > > exynos_drm_crtc_finish_pageflip(). And know that this patch makes
>> > > > sure that pended page flip event is>
>> > > completed
>> > >
>> > > > before dpms goes to off.
>> > >
>> > > You need to do vblank_put/get while you're waiting. Otherwise you
>> > > don't know for sure that flips will happen. This is especially bad as
>> > > you don't use a timeout.
>> >
>> > Understood. Right, drm_vblank_get call before wait_event and
>> > drm_vblank_put call after wait_event.
>> >
>> > Thanks,
>> > Inki Dae
>> >
>> > > Stéphane
>> > >
>> > > > Thanks,
>> > > > Inki Dae
>> > > >
>> > > >> Stéphane
>> > > >>
>> > > >> > + }
>> > > >> > +
>> > > >> >
>> > > >> > exynos_drm_fn_encoder(crtc, &mode,
>> > > >> >
>> > > >> > exynos_drm_encoder_crtc_dpms);
>> > > >> >
>> > > >> > exynos_crtc->dpms = mode;
>> > > >> >
>> > > >> > }
>> > > >> >
>> > > >> > @@ -225,6 +234,7 @@ static int exynos_drm_crtc_page_flip(struct
>> > >
>> > > drm_crtc
>> > >
>> > > >> > *crtc,
>> > > >> >
>> > > >> > spin_lock_irq(&dev->event_lock);
>> > > >> > list_add_tail(&event->base.link,
>> > > >> >
>> > > >> > &dev_priv->pageflip_event_list);
>> > > >> >
>> > > >> > + atomic_set(&exynos_crtc->pending_flip, 1);
>> > > >> >
>> > > >> > spin_unlock_irq(&dev->event_lock);
>> > > >> >
>> > > >> > crtc->fb = fb;
>> > > >> >
>> > > >> > @@ -344,6 +354,8 @@ int exynos_drm_crtc_create(struct drm_device
>> > > >> > *dev,
>> > > >> > unsigned int nr)
>> > > >> >
>> > > >> > exynos_crtc->pipe = nr;
>> > > >> > exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>> > > >> >
>> > > >> > + init_waitqueue_head(&exynos_crtc->pending_flip_queue);
>> > > >> > + atomic_set(&exynos_crtc->pending_flip, 0);
>> > > >> >
>> > > >> > exynos_crtc->plane = exynos_plane_init(dev, 1 << nr,
>> > > >> > true);
>> > > >> > if (!exynos_crtc->plane) {
>> > > >> >
>> > > >> > kfree(exynos_crtc);
>> > > >> >
>> > > >> > @@ -398,6 +410,8 @@ void exynos_drm_crtc_finish_pageflip(struct
>> > > >> > drm_device *dev, int crtc)
>> > > >> >
>> > > >> > {
>> > > >> >
>> > > >> > struct exynos_drm_private *dev_priv = dev->dev_private;
>> > > >> > struct drm_pending_vblank_event *e, *t;
>> > > >> >
>> > > >> > + struct drm_crtc *drm_crtc = dev_priv->crtc[crtc];
>> > > >> > + struct exynos_drm_crtc *exynos_crtc =
>> > >
>> > > to_exynos_crtc(drm_crtc);
>> > >
>> > > >> > struct timeval now;
>> > > >> > unsigned long flags;
>> > > >> >
>> > > >> > @@ -419,6 +433,8 @@ void exynos_drm_crtc_finish_pageflip(struct
>> > > >> > drm_device *dev, int crtc)
>> > > >> >
>> > > >> > list_move_tail(&e->base.link,
>> > > >> >
>> > > >> > &e->base.file_priv->event_list);
>> > > >> >
>> > > >> > wake_up_interruptible(&e->base.file_priv->event_w
>> > > >> > ait);
>> > > >> > drm_vblank_put(dev, crtc);
>> > > >> >
>> > > >> > + atomic_set(&exynos_crtc->pending_flip, 0);
>> > > >> > + wake_up(&exynos_crtc->pending_flip_queue);
>> > > >> >
>> > > >> > }
>> > > >> >
>> > > >> > spin_unlock_irqrestore(&dev->event_lock, flags);
>> > > >> >
>> > > >> > --
>> > > >> > 1.7.5.4
>> > > >> >
>> > > >> > _______________________________________________
>> > > >> > dri-devel mailing list
>> > > >> > dri-devel at lists.freedesktop.org
>> > > >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > >>
>> > > >> _______________________________________________
>> > > >> dri-devel mailing list
>> > > >> dri-devel at lists.freedesktop.org
>> > > >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >
>> > > _______________________________________________
>> > > dri-devel mailing list
>> > > dri-devel at lists.freedesktop.org
>> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130522/0a535db3/attachment-0001.html>
More information about the dri-devel
mailing list