| Message ID | 1428656849-5004-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/04/15 10:07, Sonika Jindal wrote: > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ceb2e61..f0bbc22 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > + bool can_position = false; > int ret; > > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > > + if (INTEL_INFO(dev)->gen >= 9) > + can_position = true; > + > ret = drm_plane_helper_check_update(plane, crtc, fb, > src, dest, clip, > DRM_PLANE_HELPER_NO_SCALING, > DRM_PLANE_HELPER_NO_SCALING, > - false, true, &state->visible); > + can_position, true, > + &state->visible); > if (ret) > return ret; > > I have discovered today that, while this allows SetCrtc and SetPlane ioctls to work with frame buffers which do not cover the plane, page flips are not that lucky and fail roughly with: [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC viewport 1920x1080+0+0. I have posted a quick IGT exerciser for this as "kms_rotation_crc: Excercise page flips with 90 degree rotation". May not be that great but shows the failure. I am not that hot on meddling with this code, nor do I feel competent to even try on my own at least. :/ Maybe just because the atomic and plane related rewrites have been going on for so long, and have multiple people involved, it all sounds pretty scary and fragile. But I think some sort of plan on how to fix this could be in order? Regards, Tvrtko
On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > On 10/04/15 10:07, Sonika Jindal wrote: > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >--- > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index ceb2e61..f0bbc22 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > struct drm_rect *dest = &state->dst; > > struct drm_rect *src = &state->src; > > const struct drm_rect *clip = &state->clip; > >+ bool can_position = false; > > int ret; > > > > crtc = crtc ? crtc : plane->crtc; > > intel_crtc = to_intel_crtc(crtc); > > > >+ if (INTEL_INFO(dev)->gen >= 9) > >+ can_position = true; > >+ > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > src, dest, clip, > > DRM_PLANE_HELPER_NO_SCALING, > > DRM_PLANE_HELPER_NO_SCALING, > >- false, true, &state->visible); > >+ can_position, true, > >+ &state->visible); > > if (ret) > > return ret; > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > ioctls to work with frame buffers which do not cover the plane, page > flips are not that lucky and fail roughly with: > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > viewport 1920x1080+0+0. Maybe I'm misunderstanding your explanation, but a framebuffer is always required to fill/cover the plane scanning out of it. What this patch is supposed to be allowing is for the primary plane to not cover the entire CRTC (since that's something that only became possible for Intel hardware on the gen9+ platforms). I.e., the primary plane is now allowed to positioned and resized to cover a subset of the CRTC area, just like "sprite" planes have always been able to. If you've got a 1080x1080 framebuffer, then it's legal to have a 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. However it is not legal to size the primary plane as 1920x1080 and use this same 1080x1080 framebuffer with any of our interfaces (setplane, setcrtc, pageflip, or atomic). Are you using ioctls/libdrm directly or are you using igt_kms helpers? IIRC, the IGT helpers will try to be extra helpful and automatically size the plane to match the framebuffer (unless you override that behavior), so that might be what's causing the confusion here. Matt > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > Excercise page flips with 90 degree rotation". May not be that great > but shows the failure. > > I am not that hot on meddling with this code, nor do I feel > competent to even try on my own at least. :/ Maybe just because the > atomic and plane related rewrites have been going on for so long, > and have multiple people involved, it all sounds pretty scary and > fragile. > > But I think some sort of plan on how to fix this could be in order? > > Regards, > > Tvrtko
On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > >--- > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > >index ceb2e61..f0bbc22 100644 > > >--- a/drivers/gpu/drm/i915/intel_display.c > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > struct drm_rect *dest = &state->dst; > > > struct drm_rect *src = &state->src; > > > const struct drm_rect *clip = &state->clip; > > >+ bool can_position = false; > > > int ret; > > > > > > crtc = crtc ? crtc : plane->crtc; > > > intel_crtc = to_intel_crtc(crtc); > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > >+ can_position = true; > > >+ > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > src, dest, clip, > > > DRM_PLANE_HELPER_NO_SCALING, > > > DRM_PLANE_HELPER_NO_SCALING, > > >- false, true, &state->visible); > > >+ can_position, true, > > >+ &state->visible); > > > if (ret) > > > return ret; > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > ioctls to work with frame buffers which do not cover the plane, page > > flips are not that lucky and fail roughly with: > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > viewport 1920x1080+0+0. > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > required to fill/cover the plane scanning out of it. What this patch is > supposed to be allowing is for the primary plane to not cover the entire > CRTC (since that's something that only became possible for Intel > hardware on the gen9+ platforms). I.e., the primary plane is now > allowed to positioned and resized to cover a subset of the CRTC area, > just like "sprite" planes have always been able to. > > If you've got a 1080x1080 framebuffer, then it's legal to have a > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > However it is not legal to size the primary plane as 1920x1080 and use > this same 1080x1080 framebuffer with any of our interfaces (setplane, > setcrtc, pageflip, or atomic). > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > IIRC, the IGT helpers will try to be extra helpful and automatically > size the plane to match the framebuffer (unless you override that > behavior), so that might be what's causing the confusion here. The problem is clear as day in drm_mode_page_flip_ioctl(): ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); if (ret) goto out; The fix should be easy; just extract the current src coordinates from the plane state and check those against the new fb size. And then hope that the plane state is really up to date. And I'm sure rotated cases will go boom in some other ways. Probably we should just switch over to using the full plane update for mmio flips to fix it. > > > Matt > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > Excercise page flips with 90 degree rotation". May not be that great > > but shows the failure. > > > > I am not that hot on meddling with this code, nor do I feel > > competent to even try on my own at least. :/ Maybe just because the > > atomic and plane related rewrites have been going on for so long, > > and have multiple people involved, it all sounds pretty scary and > > fragile. > > > > But I think some sort of plan on how to fix this could be in order? > > > > Regards, > > > > Tvrtko > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > >--- > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > >index ceb2e61..f0bbc22 100644 > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > struct drm_rect *dest = &state->dst; > > > > struct drm_rect *src = &state->src; > > > > const struct drm_rect *clip = &state->clip; > > > >+ bool can_position = false; > > > > int ret; > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > >+ can_position = true; > > > >+ > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > src, dest, clip, > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > DRM_PLANE_HELPER_NO_SCALING, > > > >- false, true, &state->visible); > > > >+ can_position, true, > > > >+ &state->visible); > > > > if (ret) > > > > return ret; > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > ioctls to work with frame buffers which do not cover the plane, page > > > flips are not that lucky and fail roughly with: > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > viewport 1920x1080+0+0. > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > required to fill/cover the plane scanning out of it. What this patch is > > supposed to be allowing is for the primary plane to not cover the entire > > CRTC (since that's something that only became possible for Intel > > hardware on the gen9+ platforms). I.e., the primary plane is now > > allowed to positioned and resized to cover a subset of the CRTC area, > > just like "sprite" planes have always been able to. > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > However it is not legal to size the primary plane as 1920x1080 and use > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > setcrtc, pageflip, or atomic). > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > IIRC, the IGT helpers will try to be extra helpful and automatically > > size the plane to match the framebuffer (unless you override that > > behavior), so that might be what's causing the confusion here. > > The problem is clear as day in drm_mode_page_flip_ioctl(): > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > if (ret) > goto out; > > The fix should be easy; just extract the current src coordinates from > the plane state and check those against the new fb size. And then hope > that the plane state is really up to date. Yep, that's the conclusion we came to once Tvrtko explained what he was seeing on IRC. I'm not sure whether non-atomic drivers have enough state setup by the default helpers to work properly. Worst case we'll just assume that a non-atomic driver won't support primary plane windowing (since none have in the past) and fall back to looking at the mode for legacy non-atomic drivers. > > And I'm sure rotated cases will go boom in some other ways. Probably > we should just switch over to using the full plane update for mmio > flips to fix it. Yeah; the core looks at a drm_plane->invert_dimensions field that's only set by omap. That should probably be updated to look at the state's rotation on atomic-capable drivers. Matt > > > > > > > Matt > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > Excercise page flips with 90 degree rotation". May not be that great > > > but shows the failure. > > > > > > I am not that hot on meddling with this code, nor do I feel > > > competent to even try on my own at least. :/ Maybe just because the > > > atomic and plane related rewrites have been going on for so long, > > > and have multiple people involved, it all sounds pretty scary and > > > fragile. > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > Regards, > > > > > > Tvrtko > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel OTC
On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: > On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > >--- > > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > >index ceb2e61..f0bbc22 100644 > > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > > struct drm_rect *dest = &state->dst; > > > > > struct drm_rect *src = &state->src; > > > > > const struct drm_rect *clip = &state->clip; > > > > >+ bool can_position = false; > > > > > int ret; > > > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > > >+ can_position = true; > > > > >+ > > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > > src, dest, clip, > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > >- false, true, &state->visible); > > > > >+ can_position, true, > > > > >+ &state->visible); > > > > > if (ret) > > > > > return ret; > > > > > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > > ioctls to work with frame buffers which do not cover the plane, page > > > > flips are not that lucky and fail roughly with: > > > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > > viewport 1920x1080+0+0. > > > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > > required to fill/cover the plane scanning out of it. What this patch is > > > supposed to be allowing is for the primary plane to not cover the entire > > > CRTC (since that's something that only became possible for Intel > > > hardware on the gen9+ platforms). I.e., the primary plane is now > > > allowed to positioned and resized to cover a subset of the CRTC area, > > > just like "sprite" planes have always been able to. > > > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > > However it is not legal to size the primary plane as 1920x1080 and use > > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > > setcrtc, pageflip, or atomic). > > > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > > IIRC, the IGT helpers will try to be extra helpful and automatically > > > size the plane to match the framebuffer (unless you override that > > > behavior), so that might be what's causing the confusion here. > > > > The problem is clear as day in drm_mode_page_flip_ioctl(): > > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > > if (ret) > > goto out; > > > > The fix should be easy; just extract the current src coordinates from > > the plane state and check those against the new fb size. And then hope > > that the plane state is really up to date. > > Yep, that's the conclusion we came to once Tvrtko explained what he was > seeing on IRC. I'm not sure whether non-atomic drivers have enough > state setup by the default helpers to work properly. Worst case we'll > just assume that a non-atomic driver won't support primary plane > windowing (since none have in the past) and fall back to looking at the > mode for legacy non-atomic drivers. > > > > > And I'm sure rotated cases will go boom in some other ways. Probably > > we should just switch over to using the full plane update for mmio > > flips to fix it. > > Yeah; the core looks at a drm_plane->invert_dimensions field that's only > set by omap. That should probably be updated to look at the state's > rotation on atomic-capable drivers. We can just look at the src coordinates. Those always match the fb orientation. > > > Matt > > > > > > > > > > > > Matt > > > > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > > Excercise page flips with 90 degree rotation". May not be that great > > > > but shows the failure. > > > > > > > > I am not that hot on meddling with this code, nor do I feel > > > > competent to even try on my own at least. :/ Maybe just because the > > > > atomic and plane related rewrites have been going on for so long, > > > > and have multiple people involved, it all sounds pretty scary and > > > > fragile. > > > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > IoTG Platform Enabling & Development > > > Intel Corporation > > > (916) 356-2795 > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: > On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: > > On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: > > > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: > > > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: > > > > > > > > > > On 10/04/15 10:07, Sonika Jindal wrote: > > > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > >--- > > > > > > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > >index ceb2e61..f0bbc22 100644 > > > > > >--- a/drivers/gpu/drm/i915/intel_display.c > > > > > >+++ b/drivers/gpu/drm/i915/intel_display.c > > > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > > > > > > struct drm_rect *dest = &state->dst; > > > > > > struct drm_rect *src = &state->src; > > > > > > const struct drm_rect *clip = &state->clip; > > > > > >+ bool can_position = false; > > > > > > int ret; > > > > > > > > > > > > crtc = crtc ? crtc : plane->crtc; > > > > > > intel_crtc = to_intel_crtc(crtc); > > > > > > > > > > > >+ if (INTEL_INFO(dev)->gen >= 9) > > > > > >+ can_position = true; > > > > > >+ > > > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, > > > > > > src, dest, clip, > > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > > DRM_PLANE_HELPER_NO_SCALING, > > > > > >- false, true, &state->visible); > > > > > >+ can_position, true, > > > > > >+ &state->visible); > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > > > I have discovered today that, while this allows SetCrtc and SetPlane > > > > > ioctls to work with frame buffers which do not cover the plane, page > > > > > flips are not that lucky and fail roughly with: > > > > > > > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC > > > > > viewport 1920x1080+0+0. > > > > > > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always > > > > required to fill/cover the plane scanning out of it. What this patch is > > > > supposed to be allowing is for the primary plane to not cover the entire > > > > CRTC (since that's something that only became possible for Intel > > > > hardware on the gen9+ platforms). I.e., the primary plane is now > > > > allowed to positioned and resized to cover a subset of the CRTC area, > > > > just like "sprite" planes have always been able to. > > > > > > > > If you've got a 1080x1080 framebuffer, then it's legal to have a > > > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. > > > > However it is not legal to size the primary plane as 1920x1080 and use > > > > this same 1080x1080 framebuffer with any of our interfaces (setplane, > > > > setcrtc, pageflip, or atomic). > > > > > > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers? > > > > IIRC, the IGT helpers will try to be extra helpful and automatically > > > > size the plane to match the framebuffer (unless you override that > > > > behavior), so that might be what's causing the confusion here. > > > > > > The problem is clear as day in drm_mode_page_flip_ioctl(): > > > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); > > > if (ret) > > > goto out; > > > > > > The fix should be easy; just extract the current src coordinates from > > > the plane state and check those against the new fb size. And then hope > > > that the plane state is really up to date. > > > > Yep, that's the conclusion we came to once Tvrtko explained what he was > > seeing on IRC. I'm not sure whether non-atomic drivers have enough > > state setup by the default helpers to work properly. Worst case we'll > > just assume that a non-atomic driver won't support primary plane > > windowing (since none have in the past) and fall back to looking at the > > mode for legacy non-atomic drivers. > > > > > > > > And I'm sure rotated cases will go boom in some other ways. Probably > > > we should just switch over to using the full plane update for mmio > > > flips to fix it. > > > > Yeah; the core looks at a drm_plane->invert_dimensions field that's only > > set by omap. That should probably be updated to look at the state's > > rotation on atomic-capable drivers. > > We can just look at the src coordinates. Those always match the fb > orientation. Can we just not bother with legacy pageflips on rotated planes? setplane works and once you rotate it kinda gets nasty anyway. The problem I see is that with legacy pageflip we also need to hack up something that doesn't look at plane->state for legacy and all that for a grand total of about 2 drivers, both getting converted to atomic. -Daniel > > > > > > > Matt > > > > > > > > > > > > > > > > > Matt > > > > > > > > > > > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc: > > > > > Excercise page flips with 90 degree rotation". May not be that great > > > > > but shows the failure. > > > > > > > > > > I am not that hot on meddling with this code, nor do I feel > > > > > competent to even try on my own at least. :/ Maybe just because the > > > > > atomic and plane related rewrites have been going on for so long, > > > > > and have multiple people involved, it all sounds pretty scary and > > > > > fragile. > > > > > > > > > > But I think some sort of plan on how to fix this could be in order? > > > > > > > > > > Regards, > > > > > > > > > > Tvrtko > > > > > > > > -- > > > > Matt Roper > > > > Graphics Software Engineer > > > > IoTG Platform Enabling & Development > > > > Intel Corporation > > > > (916) 356-2795 > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel OTC
On 07/10/15 15:19, Daniel Vetter wrote: > On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: >> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: >>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: >>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: >>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: >>>>>> >>>>>> On 10/04/15 10:07, Sonika Jindal wrote: >>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>>> index ceb2e61..f0bbc22 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, >>>>>>> struct drm_rect *dest = &state->dst; >>>>>>> struct drm_rect *src = &state->src; >>>>>>> const struct drm_rect *clip = &state->clip; >>>>>>> + bool can_position = false; >>>>>>> int ret; >>>>>>> >>>>>>> crtc = crtc ? crtc : plane->crtc; >>>>>>> intel_crtc = to_intel_crtc(crtc); >>>>>>> >>>>>>> + if (INTEL_INFO(dev)->gen >= 9) >>>>>>> + can_position = true; >>>>>>> + >>>>>>> ret = drm_plane_helper_check_update(plane, crtc, fb, >>>>>>> src, dest, clip, >>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>> - false, true, &state->visible); >>>>>>> + can_position, true, >>>>>>> + &state->visible); >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> >>>>>> >>>>>> I have discovered today that, while this allows SetCrtc and SetPlane >>>>>> ioctls to work with frame buffers which do not cover the plane, page >>>>>> flips are not that lucky and fail roughly with: >>>>>> >>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC >>>>>> viewport 1920x1080+0+0. >>>>> >>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is always >>>>> required to fill/cover the plane scanning out of it. What this patch is >>>>> supposed to be allowing is for the primary plane to not cover the entire >>>>> CRTC (since that's something that only became possible for Intel >>>>> hardware on the gen9+ platforms). I.e., the primary plane is now >>>>> allowed to positioned and resized to cover a subset of the CRTC area, >>>>> just like "sprite" planes have always been able to. >>>>> >>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a >>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. >>>>> However it is not legal to size the primary plane as 1920x1080 and use >>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane, >>>>> setcrtc, pageflip, or atomic). >>>>> >>>>> Are you using ioctls/libdrm directly or are you using igt_kms helpers? >>>>> IIRC, the IGT helpers will try to be extra helpful and automatically >>>>> size the plane to match the framebuffer (unless you override that >>>>> behavior), so that might be what's causing the confusion here. >>>> >>>> The problem is clear as day in drm_mode_page_flip_ioctl(): >>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb); >>>> if (ret) >>>> goto out; >>>> >>>> The fix should be easy; just extract the current src coordinates from >>>> the plane state and check those against the new fb size. And then hope >>>> that the plane state is really up to date. >>> >>> Yep, that's the conclusion we came to once Tvrtko explained what he was >>> seeing on IRC. I'm not sure whether non-atomic drivers have enough >>> state setup by the default helpers to work properly. Worst case we'll >>> just assume that a non-atomic driver won't support primary plane >>> windowing (since none have in the past) and fall back to looking at the >>> mode for legacy non-atomic drivers. >>> >>>> >>>> And I'm sure rotated cases will go boom in some other ways. Probably >>>> we should just switch over to using the full plane update for mmio >>>> flips to fix it. >>> >>> Yeah; the core looks at a drm_plane->invert_dimensions field that's only >>> set by omap. That should probably be updated to look at the state's >>> rotation on atomic-capable drivers. >> >> We can just look at the src coordinates. Those always match the fb >> orientation. > > Can we just not bother with legacy pageflips on rotated planes? setplane > works and once you rotate it kinda gets nasty anyway. I don't know - thought it is simple enough to make it work so why not? Just " [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip?" to allow smaller than mode planes. > The problem I see is that with legacy pageflip we also need to hack up > something that doesn't look at plane->state for legacy and all that for a > grand total of about 2 drivers, both getting converted to atomic. I'll leave the legacy/atomic/etc considerations to the experts. :) Regards, Tvrtko
On 08/10/15 09:58, Tvrtko Ursulin wrote: > On 07/10/15 15:19, Daniel Vetter wrote: >> On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote: >>> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote: >>>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote: >>>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote: >>>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote: >>>>>>> >>>>>>> On 10/04/15 10:07, Sonika Jindal wrote: >>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_display.c | 7 ++++++- >>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>>>>>>> b/drivers/gpu/drm/i915/intel_display.c >>>>>>>> index ceb2e61..f0bbc22 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct >>>>>>>> drm_plane *plane, >>>>>>>> struct drm_rect *dest = &state->dst; >>>>>>>> struct drm_rect *src = &state->src; >>>>>>>> const struct drm_rect *clip = &state->clip; >>>>>>>> + bool can_position = false; >>>>>>>> int ret; >>>>>>>> >>>>>>>> crtc = crtc ? crtc : plane->crtc; >>>>>>>> intel_crtc = to_intel_crtc(crtc); >>>>>>>> >>>>>>>> + if (INTEL_INFO(dev)->gen >= 9) >>>>>>>> + can_position = true; >>>>>>>> + >>>>>>>> ret = drm_plane_helper_check_update(plane, crtc, fb, >>>>>>>> src, dest, clip, >>>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>>> DRM_PLANE_HELPER_NO_SCALING, >>>>>>>> - false, true, &state->visible); >>>>>>>> + can_position, true, >>>>>>>> + &state->visible); >>>>>>>> if (ret) >>>>>>>> return ret; >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> I have discovered today that, while this allows SetCrtc and SetPlane >>>>>>> ioctls to work with frame buffers which do not cover the plane, page >>>>>>> flips are not that lucky and fail roughly with: >>>>>>> >>>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC >>>>>>> viewport 1920x1080+0+0. >>>>>> >>>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is >>>>>> always >>>>>> required to fill/cover the plane scanning out of it. What this >>>>>> patch is >>>>>> supposed to be allowing is for the primary plane to not cover the >>>>>> entire >>>>>> CRTC (since that's something that only became possible for Intel >>>>>> hardware on the gen9+ platforms). I.e., the primary plane is now >>>>>> allowed to positioned and resized to cover a subset of the CRTC area, >>>>>> just like "sprite" planes have always been able to. >>>>>> >>>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a >>>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT. >>>>>> However it is not legal to size the primary plane as 1920x1080 and >>>>>> use >>>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane, >>>>>> setcrtc, pageflip, or atomic). >>>>>> >>>>>> Are you using ioctls/libdrm directly or are you using igt_kms >>>>>> helpers? >>>>>> IIRC, the IGT helpers will try to be extra helpful and automatically >>>>>> size the plane to match the framebuffer (unless you override that >>>>>> behavior), so that might be what's causing the confusion here. >>>>> >>>>> The problem is clear as day in drm_mode_page_flip_ioctl(): >>>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, >>>>> fb); >>>>> if (ret) >>>>> goto out; >>>>> >>>>> The fix should be easy; just extract the current src coordinates from >>>>> the plane state and check those against the new fb size. And then hope >>>>> that the plane state is really up to date. >>>> >>>> Yep, that's the conclusion we came to once Tvrtko explained what he was >>>> seeing on IRC. I'm not sure whether non-atomic drivers have enough >>>> state setup by the default helpers to work properly. Worst case we'll >>>> just assume that a non-atomic driver won't support primary plane >>>> windowing (since none have in the past) and fall back to looking at the >>>> mode for legacy non-atomic drivers. >>>> >>>>> >>>>> And I'm sure rotated cases will go boom in some other ways. Probably >>>>> we should just switch over to using the full plane update for mmio >>>>> flips to fix it. >>>> >>>> Yeah; the core looks at a drm_plane->invert_dimensions field that's >>>> only >>>> set by omap. That should probably be updated to look at the state's >>>> rotation on atomic-capable drivers. >>> >>> We can just look at the src coordinates. Those always match the fb >>> orientation. >> >> Can we just not bother with legacy pageflips on rotated planes? setplane >> works and once you rotate it kinda gets nasty anyway. > > I don't know - thought it is simple enough to make it work so why not? > Just " [PATCH] drm/i915: Consider plane rotation when calculating stride > in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb > against plane size rather than CRTC mode for pageflip?" to allow smaller > than mode planes. > >> The problem I see is that with legacy pageflip we also need to hack up >> something that doesn't look at plane->state for legacy and all that for a >> grand total of about 2 drivers, both getting converted to atomic. > > I'll leave the legacy/atomic/etc considerations to the experts. :) Are we sure any efforts to support rotation in legacy page flips is not worth it? So far there were three patches for this: Plane programming fix (very simple) and an IGT test case (simple as well) from me, and a sub-crtc size plane support from Matt. It kind of remained hanging a bit so I think it would be good to make a definitive decision. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ceb2e61..f0bbc22 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; + bool can_position = false; int ret; crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); + if (INTEL_INFO(dev)->gen >= 9) + can_position = true; + ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, - false, true, &state->visible); + can_position, true, + &state->visible); if (ret) return ret;