Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

On Wed, Jan 16, 2013 at 10:43:15AM +0100, Daniel Vetter wrote:
> On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
> >> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding
> >> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> >> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
> >> > +{
> >> > +       struct drm_crtc *crtc;
> >> > +
> >> > +       list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> >> > +               tegra_dc_cancel_page_flip(crtc, file);
> >> > +}
> >> > +
> >>
> >> Why that? If userspace dies while a flip is outstanding, it's imo ok
> >> to execute it - otherwise there might be an accounting mismatch if the
> >> hw still scans out the old fb, but drm believes the new one is used.
> >> Or do I miss something?
> >
> > I looked at the shmobile driver for inspiration and they do this as
> > well. Doing so seemed reasonable since there'd be no file to deliver the
> > event to.
> 
> Hm, is the code in drm_events_release not good enough? And if it's
> buggy, we need to fix it. Also adding Laurent to figure out why he
> added that code in shmob ...

drm_events_release() should be enough to clean up the events, but I
suspect the reason why Laurent put that code in was that the drm_crtc
private data still has a reference to the event and needs to clear it.
Otherwise the next page flip won't be scheduled because .page_flip()
would return -EBUSY.

However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
could both be simplified a lot and just set their event to NULL. Then
again, maybe keeping a separate reference isn't all that useful. Maybe
the better thing to do here is iterate over the list of pending VBLANK
events in *_finish_page_flip() and process each of them? That would
allow more than one user-space process to queue page flips.

> >> The reason I've skimmed through the patches is to check for
> >> implications with my modeset locking rework. Review on that would be
> >> highly appreciated ...
> >
> > I'm not sure how suited I am for review given my limited experience, but
> > I'll see if I can make some time to take a look.
> 
> The commit message should nicely explain why I've picked the design
> and the various implications for drivers. So just checking whether
> anything collides with your upcoming stuff would be good ...

Okay, I'll take a closer look.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]