[PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Daniel Vetter]

On Thu, May 15, 2014 at 12:42 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Thu, May 15, 2014 at 12:10:16PM +0200, Daniel Vetter wrote:
>> On Thu, May 15, 2014 at 9:34 AM, Thierry Reding
>> <thierry.reding at gmail.com> wrote:
>> > This seems slightly backwards. Since drm_vblank_get() is what's being
>> > deprecated here, wouldn't it make more sense to write
>> > drm_crtc_vblank_get() in terms of struct drm_crtc and make
>> > drm_vblank_get() call that instead? I can't seem to find a helper to get
>> > the CRTC from an index, but it seems like that wouldn't be hard to do.
>>
>> Two reasons against this:
>> - More ugly churn since it's a flag day, and when handling vblank
>> refactorings what I _definitely_ want to avoid is whole-subsystem wide
>> flag days.
>> - drm_crtc_ is the common prefix used by many of the crtc functions.
>>
>> What I actually forgotten to do is drop the dev parameter, we can fish
>> that out of the crtc. Then it should be even more obvious that this is
>> a crtc function and rightfully deserve the drm_crtc_ prefix ;-)
>
> I think you misunderstood what I was saying. What I proposed wasn't that
> drm_vblank_get() was replaced by a new implementation with different
> signature. Rather my suggestion was to implement the old
> drm_vblank_get() by calling drm_crtc_vblank_get() rather than the other
> way around.
>
> Something like this:
>
>         int drm_crtc_vblank_get(struct drm_crtc *crtc)
>         {
>                 new code using CRTC
>         }
>
>         int drm_vblank_get(struct drm_device *drm, int crtc)
>         {
>                 struct drm_crtc *c = drm_crtc_from_index(crtc);
>
>                 return drm_crtc_vblank_get(c);
>         }

As long as the actual code doesn't deal in real drm_crtcs that imo
makes little sense. It's really just the interface shim to start the
long journey into a saner world ;-)

>> > I guess it doesn't matter all that much either way, though, since we
>> > could equally well make that change when drm_vblank_get() is dropped, in
>> > which case at least there's no longer a need for the reverse lookup.
>>
>> Yeah, the reverse lookup is something I want to add later on
>> eventually. But that requires more thought since it only makes sense
>> if we also switch the driver callbacks for vblank_enable/disable over.
>
> On that note, is there a plan to move the vblank fields out of the DRM
> device and into drm_crtc as well? That seems like a logical next step
> since presumably every CRTC can handle it's own vblank events itself.

Yeah, I think that's where we eventually want to go to. The problem is
that the vblank code is deeply intertwined with legacy
user-mode-setting drivers. We might need to do a copy-paste of
drm_irq.c for kms drivers into a new drm_crtc_vblank.c file which
exclusively deals with drm_crtcs. But I don't have any clear idea yet
how to make that transition happen, hence this patch to start with
something small and something we clearly want.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch