| Message ID | 1457539058-5782-2-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote: > From: Micah Fedke <micah.fedke@collabora.co.uk> > > This function uses an intel-specific ioctl to fetch a mapping between pipes and > crtc ids, but this technique is outdated as the crtc id is now always > equivalent to its index in the array of crtcs returned by the kernel. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> Ok, there's 2 use-cases for this one really: - intel hw specific stuff, like CS scanline waits (like in the overlay, or other places) - to figure out the pipe ID for the vblank ioctl The former is i915 specific, the later is generic. I think the correct way to fix this is to have a helper to encode a drmCrtc into the vblank flags. That's needed anyway since the rules are silly: First a flag to select between pipe 0 and 1, then a bitfield for 2 and later. Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or similar) we can roll it out to all the places that use the vblank ioctl. Or which check vblank events (from atomic or legacy page flips), and should be left with just the i915-specific usage. A bit more work, but I think cleaner in intent. Thoughts? -Daniel > --- > > Changes in v2: None > > lib/igt_kms.c | 33 ++++++++++++++++++++++++--------- > 1 file changed, 24 insertions(+), 9 deletions(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 9f18aef72ea0..1d9acce31676 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode) > * @fd: DRM fd > * @crtc_id: DRM CRTC id > * > - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly > - * to an enum pipe value used in other helper functions. > + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index > + * is the equivalent of the pipe id. This value maps directly to an enum pipe > + * value used in other helper functions. Returns 0 if the index could not be > + * determined. > */ > + > int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id) > { > - struct drm_i915_get_pipe_from_crtc_id pfci; > - int ret; > + drmModeRes *res; > + drmModeCrtc *crtc; > + int i, cur_id; > + > + res = drmModeGetResources(fd); > + igt_assert(res); > + > + for (i = 0; i < res->count_crtcs; i++) { > + crtc = drmModeGetCrtc(fd, res->crtcs[i]); > + igt_assert(crtc); > + cur_id = crtc->crtc_id; > + drmModeFreeCrtc(crtc); > + if (cur_id == crtc_id) > + break; > + } > + > + drmModeFreeResources(res); > > - memset(&pfci, 0, sizeof(pfci)); > - pfci.crtc_id = crtc_id; > - ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci); > - igt_assert(ret == 0); > + igt_assert(i < res->count_crtcs); > > - return pfci.pipe; > + return i; > } > > /** > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 18 April 2016 at 14:09, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote: >> From: Micah Fedke <micah.fedke@collabora.co.uk> >> >> This function uses an intel-specific ioctl to fetch a mapping between pipes and >> crtc ids, but this technique is outdated as the crtc id is now always >> equivalent to its index in the array of crtcs returned by the kernel. >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Ok, there's 2 use-cases for this one really: > - intel hw specific stuff, like CS scanline waits (like in the overlay, or > other places) > - to figure out the pipe ID for the vblank ioctl > > The former is i915 specific, the later is generic. I think the correct way > to fix this is to have a helper to encode a drmCrtc into the vblank flags. > That's needed anyway since the rules are silly: First a flag to select > between pipe 0 and 1, then a bitfield for 2 and later. > > Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or > similar) we can roll it out to all the places that use the vblank ioctl. > Or which check vblank events (from atomic or legacy page flips), and > should be left with just the i915-specific usage. > > A bit more work, but I think cleaner in intent. > > Thoughts? Sounds good in principle, will give it a try. Thanks, Tomeu > -Daniel > >> --- >> >> Changes in v2: None >> >> lib/igt_kms.c | 33 ++++++++++++++++++++++++--------- >> 1 file changed, 24 insertions(+), 9 deletions(-) >> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> index 9f18aef72ea0..1d9acce31676 100644 >> --- a/lib/igt_kms.c >> +++ b/lib/igt_kms.c >> @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode) >> * @fd: DRM fd >> * @crtc_id: DRM CRTC id >> * >> - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly >> - * to an enum pipe value used in other helper functions. >> + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index >> + * is the equivalent of the pipe id. This value maps directly to an enum pipe >> + * value used in other helper functions. Returns 0 if the index could not be >> + * determined. >> */ >> + >> int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id) >> { >> - struct drm_i915_get_pipe_from_crtc_id pfci; >> - int ret; >> + drmModeRes *res; >> + drmModeCrtc *crtc; >> + int i, cur_id; >> + >> + res = drmModeGetResources(fd); >> + igt_assert(res); >> + >> + for (i = 0; i < res->count_crtcs; i++) { >> + crtc = drmModeGetCrtc(fd, res->crtcs[i]); >> + igt_assert(crtc); >> + cur_id = crtc->crtc_id; >> + drmModeFreeCrtc(crtc); >> + if (cur_id == crtc_id) >> + break; >> + } >> + >> + drmModeFreeResources(res); >> >> - memset(&pfci, 0, sizeof(pfci)); >> - pfci.crtc_id = crtc_id; >> - ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci); >> - igt_assert(ret == 0); >> + igt_assert(i < res->count_crtcs); >> >> - return pfci.pipe; >> + return i; >> } >> >> /** >> -- >> 2.5.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > 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
On Mon, Apr 18, 2016 at 02:20:43PM +0200, Tomeu Vizoso wrote: > On 18 April 2016 at 14:09, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote: > >> From: Micah Fedke <micah.fedke@collabora.co.uk> > >> > >> This function uses an intel-specific ioctl to fetch a mapping between pipes and > >> crtc ids, but this technique is outdated as the crtc id is now always > >> equivalent to its index in the array of crtcs returned by the kernel. > >> > >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > > > Ok, there's 2 use-cases for this one really: > > - intel hw specific stuff, like CS scanline waits (like in the overlay, or > > other places) > > - to figure out the pipe ID for the vblank ioctl > > > > The former is i915 specific, the later is generic. I think the correct way > > to fix this is to have a helper to encode a drmCrtc into the vblank flags. > > That's needed anyway since the rules are silly: First a flag to select > > between pipe 0 and 1, then a bitfield for 2 and later. > > > > Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or > > similar) we can roll it out to all the places that use the vblank ioctl. > > Or which check vblank events (from atomic or legacy page flips), and > > should be left with just the i915-specific usage. > > > > A bit more work, but I think cleaner in intent. > > > > Thoughts? > > Sounds good in principle, will give it a try. Otoh, sounds like busywork on 2nd consideration. Mostly I just thought of this because of the overlay patch. But really, numbers match exactly between vblank pipe encoding and i915 pipe, so might as well go with this one patch here. Applied 1/2 (and only that one) to igt, thanks. -Daniel > > Thanks, > > Tomeu > > > -Daniel > > > >> --- > >> > >> Changes in v2: None > >> > >> lib/igt_kms.c | 33 ++++++++++++++++++++++++--------- > >> 1 file changed, 24 insertions(+), 9 deletions(-) > >> > >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c > >> index 9f18aef72ea0..1d9acce31676 100644 > >> --- a/lib/igt_kms.c > >> +++ b/lib/igt_kms.c > >> @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode) > >> * @fd: DRM fd > >> * @crtc_id: DRM CRTC id > >> * > >> - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly > >> - * to an enum pipe value used in other helper functions. > >> + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index > >> + * is the equivalent of the pipe id. This value maps directly to an enum pipe > >> + * value used in other helper functions. Returns 0 if the index could not be > >> + * determined. > >> */ > >> + > >> int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id) > >> { > >> - struct drm_i915_get_pipe_from_crtc_id pfci; > >> - int ret; > >> + drmModeRes *res; > >> + drmModeCrtc *crtc; > >> + int i, cur_id; > >> + > >> + res = drmModeGetResources(fd); > >> + igt_assert(res); > >> + > >> + for (i = 0; i < res->count_crtcs; i++) { > >> + crtc = drmModeGetCrtc(fd, res->crtcs[i]); > >> + igt_assert(crtc); > >> + cur_id = crtc->crtc_id; > >> + drmModeFreeCrtc(crtc); > >> + if (cur_id == crtc_id) > >> + break; > >> + } > >> + > >> + drmModeFreeResources(res); > >> > >> - memset(&pfci, 0, sizeof(pfci)); > >> - pfci.crtc_id = crtc_id; > >> - ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci); > >> - igt_assert(ret == 0); > >> + igt_assert(i < res->count_crtcs); > >> > >> - return pfci.pipe; > >> + return i; > >> } > >> > >> /** > >> -- > >> 2.5.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > 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
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 9f18aef72ea0..1d9acce31676 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode) * @fd: DRM fd * @crtc_id: DRM CRTC id * - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly - * to an enum pipe value used in other helper functions. + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index + * is the equivalent of the pipe id. This value maps directly to an enum pipe + * value used in other helper functions. Returns 0 if the index could not be + * determined. */ + int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id) { - struct drm_i915_get_pipe_from_crtc_id pfci; - int ret; + drmModeRes *res; + drmModeCrtc *crtc; + int i, cur_id; + + res = drmModeGetResources(fd); + igt_assert(res); + + for (i = 0; i < res->count_crtcs; i++) { + crtc = drmModeGetCrtc(fd, res->crtcs[i]); + igt_assert(crtc); + cur_id = crtc->crtc_id; + drmModeFreeCrtc(crtc); + if (cur_id == crtc_id) + break; + } + + drmModeFreeResources(res); - memset(&pfci, 0, sizeof(pfci)); - pfci.crtc_id = crtc_id; - ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci); - igt_assert(ret == 0); + igt_assert(i < res->count_crtcs); - return pfci.pipe; + return i; } /**