| Message ID | 20170503162837.19747-4-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lucas, On 3 May 2017 at 17:28, Lucas Stach <l.stach@pengutronix.de> wrote: > int available_pres = ipu_prg_max_active_channels(); > int i; > > + /* > + * We are going over the planes in 2 passes: first we assign PREs to > + * planes with a tiling modifier, which need the PREs to resolve into > + * linear. Any failure to assign a PRE there is fatal. In the second > + * pass we try to assign PREs to linear FBs, to improve memory access > + * patterns for them. Failure at this point is non-fatal, as we can > + * scan out linear FBs without a PRE. > + */ > for_each_plane_in_state(state, plane, plane_state, i) { > - struct ipu_plane_state *ipu_state = > - to_ipu_plane_state(plane_state); > - struct ipu_plane *ipu_plane = to_ipu_plane(plane); > + ipu_state = to_ipu_plane_state(plane_state); > + ipu_plane = to_ipu_plane(plane); > + > + if (!plane_state->fb) { > + ipu_state->use_pre = false; > + continue; > + } > + > + if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) || > + plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR) > + continue; > + > + if (!ipu_prg_present(ipu_plane->ipu) || !available_pres) > + return -EINVAL; What about planes which aren't present in this commit, but are still taking up a PRE unit? Will they have their PRE stolen, or am I missing something? Cheers, Daniel
Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone: > Hi Lucas, > > On 3 May 2017 at 17:28, Lucas Stach <l.stach@pengutronix.de> wrote: > > int available_pres = ipu_prg_max_active_channels(); > > int i; > > > > + /* > > + * We are going over the planes in 2 passes: first we assign PREs to > > + * planes with a tiling modifier, which need the PREs to resolve into > > + * linear. Any failure to assign a PRE there is fatal. In the second > > + * pass we try to assign PREs to linear FBs, to improve memory access > > + * patterns for them. Failure at this point is non-fatal, as we can > > + * scan out linear FBs without a PRE. > > + */ > > for_each_plane_in_state(state, plane, plane_state, i) { > > - struct ipu_plane_state *ipu_state = > > - to_ipu_plane_state(plane_state); > > - struct ipu_plane *ipu_plane = to_ipu_plane(plane); > > + ipu_state = to_ipu_plane_state(plane_state); > > + ipu_plane = to_ipu_plane(plane); > > + > > + if (!plane_state->fb) { > > + ipu_state->use_pre = false; > > + continue; > > + } > > + > > + if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) || > > + plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR) > > + continue; > > + > > + if (!ipu_prg_present(ipu_plane->ipu) || !available_pres) > > + return -EINVAL; > > What about planes which aren't present in this commit, but are still > taking up a PRE unit? Will they have their PRE stolen, or am I missing > something? Yes, the plane->PRE assignment is configurable by matching different AXI IDs in the PRG. So what's happening here is that we basically construct a new assignment for each commit. Planes without a assigned PRE will revert back to pass-through mode in the PRG on plane commit or plane disable. Regards, Lucas
Hi, On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote: > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone: >> What about planes which aren't present in this commit, but are still >> taking up a PRE unit? Will they have their PRE stolen, or am I missing >> something? > > Yes, the plane->PRE assignment is configurable by matching different AXI > IDs in the PRG. So what's happening here is that we basically construct > a new assignment for each commit. Planes without a assigned PRE will > revert back to pass-through mode in the PRG on plane commit or plane > disable. So those other planes are fine/untouched, because the PRE has already resolved to linear? Specifically if plane A has a tiled FB, then userspace does a commit which _only_ contains plane B (also with a tiled FB), and plane B steals the PRE that was previously assigned to plane A, then plane A continues displaying just fine? Sorry for the stupid questions. :) Cheers, Daniel
Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone: > Hi, > > On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone: > >> What about planes which aren't present in this commit, but are still > >> taking up a PRE unit? Will they have their PRE stolen, or am I missing > >> something? > > > > Yes, the plane->PRE assignment is configurable by matching different AXI > > IDs in the PRG. So what's happening here is that we basically construct > > a new assignment for each commit. Planes without a assigned PRE will > > revert back to pass-through mode in the PRG on plane commit or plane > > disable. > > So those other planes are fine/untouched, because the PRE has already > resolved to linear? Specifically if plane A has a tiled FB, then > userspace does a commit which _only_ contains plane B (also with a > tiled FB), and plane B steals the PRE that was previously assigned to > plane A, then plane A continues displaying just fine? Sorry for the > stupid questions. :) If userspace does a commit with only plane B it's going to disable plane A, right? The PRG/PRE register sets are double buffered (or at least we always use them in this mode). So if plane B steals the PRE from plane A, which is going to get disabled, plane A will continue to use the PRE until the end of frame, then the register set will be latched and the PRE is switched to plane B for the next frame. Regards, Lucas
On Thu, May 4, 2017 at 11:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone: >> On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote: >> > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone: >> >> What about planes which aren't present in this commit, but are still >> >> taking up a PRE unit? Will they have their PRE stolen, or am I missing >> >> something? >> > >> > Yes, the plane->PRE assignment is configurable by matching different AXI >> > IDs in the PRG. So what's happening here is that we basically construct >> > a new assignment for each commit. Planes without a assigned PRE will >> > revert back to pass-through mode in the PRG on plane commit or plane >> > disable. >> >> So those other planes are fine/untouched, because the PRE has already >> resolved to linear? Specifically if plane A has a tiled FB, then >> userspace does a commit which _only_ contains plane B (also with a >> tiled FB), and plane B steals the PRE that was previously assigned to >> plane A, then plane A continues displaying just fine? Sorry for the >> stupid questions. :) > > If userspace does a commit with only plane B it's going to disable plane > A, right? The PRG/PRE register sets are double buffered (or at least we > always use them in this mode). So if plane B steals the PRE from plane > A, which is going to get disabled, plane A will continue to use the PRE > until the end of frame, then the register set will be latched and the > PRE is switched to plane B for the next frame. Atomic is incremental, if a property is unchanged it's supposed to keep as-is. Same with entire objects. What's more, did you wire up the fb->dirty hook to make sure this stuff gets flushed correctly for frontbuffer rendering? Userspace is allowed to render into the current frontbuffer using gl, call all the flush/clear stuff and then call the DIRTY_FB ioctl, and things are supposed to show up on the screen. There's some ideas from the simple pipe helpers to essentially unify frontbuffer flushing and treat it like any atomic commit. tldr; you need a PRE for all planes, you're looking for drm_atomic_add_affected_planes() to make sure all planes are in the update. -Daniel
Am Donnerstag, den 04.05.2017, 11:17 +0200 schrieb Daniel Vetter: > On Thu, May 4, 2017 at 11:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone: > >> On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote: > >> > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone: > >> >> What about planes which aren't present in this commit, but are still > >> >> taking up a PRE unit? Will they have their PRE stolen, or am I missing > >> >> something? > >> > > >> > Yes, the plane->PRE assignment is configurable by matching different AXI > >> > IDs in the PRG. So what's happening here is that we basically construct > >> > a new assignment for each commit. Planes without a assigned PRE will > >> > revert back to pass-through mode in the PRG on plane commit or plane > >> > disable. > >> > >> So those other planes are fine/untouched, because the PRE has already > >> resolved to linear? Specifically if plane A has a tiled FB, then > >> userspace does a commit which _only_ contains plane B (also with a > >> tiled FB), and plane B steals the PRE that was previously assigned to > >> plane A, then plane A continues displaying just fine? Sorry for the > >> stupid questions. :) > > > > If userspace does a commit with only plane B it's going to disable plane > > A, right? The PRG/PRE register sets are double buffered (or at least we > > always use them in this mode). So if plane B steals the PRE from plane > > A, which is going to get disabled, plane A will continue to use the PRE > > until the end of frame, then the register set will be latched and the > > PRE is switched to plane B for the next frame. > > Atomic is incremental, if a property is unchanged it's supposed to > keep as-is. Same with entire objects. > > What's more, did you wire up the fb->dirty hook to make sure this > stuff gets flushed correctly for frontbuffer rendering? Userspace is > allowed to render into the current frontbuffer using gl, call all the > flush/clear stuff and then call the DIRTY_FB ioctl, and things are > supposed to show up on the screen. There's some ideas from the simple > pipe helpers to essentially unify frontbuffer flushing and treat it > like any atomic commit. The PREs are stream processors, they are only prefetching (and resolving) a few scanlines to internal SRAM. Any rendering to the frontbuffer will properly show up without any additional dirty tracking. You can think of the PREs as slightly bigger display FIFOs. > tldr; you need a PRE for all planes, you're looking for > drm_atomic_add_affected_planes() to make sure all planes are in the > update. Thanks, I'll look at this. Regards, Lucas
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 50add2f9e250..d21e7db95979 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -266,6 +266,7 @@ static int imx_drm_bind(struct device *dev) drm->mode_config.max_height = 4096; drm->mode_config.funcs = &imx_drm_mode_config_funcs; drm->mode_config.helper_private = &imx_drm_mode_config_helpers; + drm->mode_config.allow_fb_modifiers = true; drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 705ca93847ff..44593dd6ba31 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -527,7 +527,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, drm_rect_height(&state->src) >> 16, state->fb->pitches[0], state->fb->format->format, - 0, + state->fb->modifier, &eba); } @@ -673,17 +673,62 @@ int ipu_planes_assign_pre(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_plane_state *plane_state; + struct ipu_plane_state *ipu_state; + struct ipu_plane *ipu_plane; struct drm_plane *plane; int available_pres = ipu_prg_max_active_channels(); int i; + /* + * We are going over the planes in 2 passes: first we assign PREs to + * planes with a tiling modifier, which need the PREs to resolve into + * linear. Any failure to assign a PRE there is fatal. In the second + * pass we try to assign PREs to linear FBs, to improve memory access + * patterns for them. Failure at this point is non-fatal, as we can + * scan out linear FBs without a PRE. + */ for_each_plane_in_state(state, plane, plane_state, i) { - struct ipu_plane_state *ipu_state = - to_ipu_plane_state(plane_state); - struct ipu_plane *ipu_plane = to_ipu_plane(plane); + ipu_state = to_ipu_plane_state(plane_state); + ipu_plane = to_ipu_plane(plane); + + if (!plane_state->fb) { + ipu_state->use_pre = false; + continue; + } + + if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) || + plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR) + continue; + + if (!ipu_prg_present(ipu_plane->ipu) || !available_pres) + return -EINVAL; + + if (!ipu_prg_format_supported(ipu_plane->ipu, + plane_state->fb->format->format, + plane_state->fb->modifier)) + return -EINVAL; + + ipu_state->use_pre = true; + available_pres--; + } + + for_each_plane_in_state(state, plane, plane_state, i) { + ipu_state = to_ipu_plane_state(plane_state); + ipu_plane = to_ipu_plane(plane); + + if (!plane_state->fb) { + ipu_state->use_pre = false; + continue; + } + + if ((plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) && + plane_state->fb->modifier != DRM_FORMAT_MOD_LINEAR) + continue; + + /* make sure that modifier is initialized */ + plane_state->fb->modifier = DRM_FORMAT_MOD_LINEAR; if (ipu_prg_present(ipu_plane->ipu) && available_pres && - plane_state->fb && ipu_prg_format_supported(ipu_plane->ipu, plane_state->fb->format->format, plane_state->fb->modifier)) {
This adds FB modifier support for the Vivante single buffer tiled formats, when the PRG/PRE engines are present. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/imx/imx-drm-core.c | 1 + drivers/gpu/drm/imx/ipuv3-plane.c | 55 ++++++++++++++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 5 deletions(-)