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