| Message ID | 1461164389-15726-5-git-send-email-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 April 2016 at 16:59, <robert.foss@collabora.com> wrote: > From: Robert Foss <robert.foss@collabora.com> > > Avoid moving the cursor plane when on non-intel hardware. > Running the move block on hardware with more than IGT_PLANE_CURSOR > number of planes causes planes do be zeroed out. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > --- > lib/igt_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 3f953ec..522ede5 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) > * only 1 sprite, that's the wrong slot and we need to > * move it down. > */ > - if (p != IGT_PLANE_CURSOR) { > + if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) { IS_INTEL takes a devid, not a fd, and its meaning is that the devid corresponds to known hardware made by Intel. In this case we probably want to know if the DRM driver is i915, and for that you have is_i915_device(fd). Regards, Tomeu > pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR]; > pipe->planes[p].index = p; > memset(&pipe->planes[IGT_PLANE_CURSOR], 0, > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote: > From: Robert Foss <robert.foss@collabora.com> > > Avoid moving the cursor plane when on non-intel hardware. > Running the move block on hardware with more than IGT_PLANE_CURSOR > number of planes causes planes do be zeroed out. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > --- > lib/igt_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 3f953ec..522ede5 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) > * only 1 sprite, that's the wrong slot and we need to > * move it down. > */ > - if (p != IGT_PLANE_CURSOR) { > + if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) { Reading this again, isn't the problem that on some hw there's simply no cursor plane? With universal planes the cursor plane should alias with some of the real planes, and we simply need to make sure that we get that aliasing right. But if there's no cursor registered, well there's no cursor ... -Daniel > pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR]; > pipe->planes[p].index = p; > memset(&pipe->planes[IGT_PLANE_CURSOR], 0, > -- > 2.5.0 >
On 04/21/2016 10:48 AM, Daniel Vetter wrote: > On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote: >> From: Robert Foss <robert.foss@collabora.com> >> >> Avoid moving the cursor plane when on non-intel hardware. >> Running the move block on hardware with more than IGT_PLANE_CURSOR >> number of planes causes planes do be zeroed out. >> >> Signed-off-by: Robert Foss <robert.foss@collabora.com> >> --- >> lib/igt_kms.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> index 3f953ec..522ede5 100644 >> --- a/lib/igt_kms.c >> +++ b/lib/igt_kms.c >> @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) >> * only 1 sprite, that's the wrong slot and we need to >> * move it down. >> */ >> - if (p != IGT_PLANE_CURSOR) { >> + if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) { > Reading this again, isn't the problem that on some hw there's simply no > cursor plane? With universal planes the cursor plane should alias with > some of the real planes, and we simply need to make sure that we get that > aliasing right. > > But if there's no cursor registered, well there's no cursor ... > -Daniel From my point of view the problem is differentiating between the cases when IGT_PLANE_CURSOR is aliasing with IGT_PLANE_4 (IGT_PLANE_3+1). IGT_PLANE_CURSOR is assumed to be the last plane in the if statement, which isn't the case for platform with more than 4 planes. So identifying which platforms this assumption is true (IS_INTEL()) and only proceeding for those platforms seemed like reasonable fix. Maybe a more generic solution is preferable. >> pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR]; >> pipe->planes[p].index = p; >> memset(&pipe->planes[IGT_PLANE_CURSOR], 0, >> -- >> 2.5.0 >>
On Thu, Apr 21, 2016 at 01:24:53PM -0400, Robert Foss wrote: > > > On 04/21/2016 10:48 AM, Daniel Vetter wrote: > >On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote: > >>From: Robert Foss <robert.foss@collabora.com> > >> > >>Avoid moving the cursor plane when on non-intel hardware. > >>Running the move block on hardware with more than IGT_PLANE_CURSOR > >>number of planes causes planes do be zeroed out. > >> > >>Signed-off-by: Robert Foss <robert.foss@collabora.com> > >>--- > >> lib/igt_kms.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/lib/igt_kms.c b/lib/igt_kms.c > >>index 3f953ec..522ede5 100644 > >>--- a/lib/igt_kms.c > >>+++ b/lib/igt_kms.c > >>@@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) > >> * only 1 sprite, that's the wrong slot and we need to > >> * move it down. > >> */ > >>- if (p != IGT_PLANE_CURSOR) { > >>+ if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) { > >Reading this again, isn't the problem that on some hw there's simply no > >cursor plane? With universal planes the cursor plane should alias with > >some of the real planes, and we simply need to make sure that we get that > >aliasing right. > > > >But if there's no cursor registered, well there's no cursor ... > >-Daniel > From my point of view the problem is differentiating between the cases when > IGT_PLANE_CURSOR is aliasing with IGT_PLANE_4 (IGT_PLANE_3+1). > > IGT_PLANE_CURSOR is assumed to be the last plane in the if statement, > which isn't the case for platform with more than 4 planes. That's why you need to add more IGT_PLANE_x defines, to make sure _CURSOR and the last one don't accidentally alias. Because if they do, we have a problem. -Daniel > > So identifying which platforms this assumption is true (IS_INTEL()) and > only proceeding for those platforms seemed like reasonable fix. > Maybe a more generic solution is preferable. > >> pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR]; > >> pipe->planes[p].index = p; > >> memset(&pipe->planes[IGT_PLANE_CURSOR], 0, > >>-- > >>2.5.0 > >> >
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 3f953ec..522ede5 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) * only 1 sprite, that's the wrong slot and we need to * move it down. */ - if (p != IGT_PLANE_CURSOR) { + if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) { pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR]; pipe->planes[p].index = p; memset(&pipe->planes[IGT_PLANE_CURSOR], 0,