| Message ID | 1351188354-24233-47-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Send completion events when the atomic modesetting operations has > finished succesfully. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I have to admit I'm not on top of the latest ioctl/interface discussion, but one new requirement for the modeset (not the pageflip part) of the all this atomic stuff I've discovered is that the kernel needs to be able to select the crtcs for each output completely unrestricted. I think userspace should simply pass in abstract crtc ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual crtcs it has selected. We can't do that remapping internally because the crtc abstraction is leaky: - wait_for_vblank requires the pipe id, which could then change on every modeset - similarly userspace doing MI_WAIT for scanlines needs to know the actual hw pipe in use by a crtc. And current userspace presumes that the mapping between crtc->pipe doesn't change. An example why the kernel needs to pick the crtc for userspace: - on ivb only pipe A has a 7x5 panel fitter, so usually you want to put the panel on the first crtc - but if you run in a 3 pipe configuration and have an eDP panel, it's better to put the eDP output on pipe C, since then pipes A&B will have full 4-lane fdi links to the pch ports (instead of otherwise only 2 lanes each on links B&C) - similarly when we have a 3 pipe configuration with all encoders on the pch, we need to move the mode with the highest dotclock to pipe A (since that's the only one which will have 4 lanes, pipe B&C will only have 2 each). - afaik these kind of asymmetric constraints won't disappear anytime soon, haswell definitely still has some. Cheers, Daniel
On Thu, 1 Nov 2012 12:12:35 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Send completion events when the atomic modesetting operations has > > finished succesfully. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > I have to admit I'm not on top of the latest ioctl/interface > discussion, but one new requirement for the modeset (not the pageflip > part) of the all this atomic stuff I've discovered is that the kernel > needs to be able to select the crtcs for each output completely > unrestricted. I think userspace should simply pass in abstract crtc > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > crtcs it has selected. > > We can't do that remapping internally because the crtc abstraction is leaky: > - wait_for_vblank requires the pipe id, which could then change on every modeset > - similarly userspace doing MI_WAIT for scanlines needs to know the > actual hw pipe in use by a crtc. > And current userspace presumes that the mapping between crtc->pipe > doesn't change. > > An example why the kernel needs to pick the crtc for userspace: > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to > put the panel on the first crtc > - but if you run in a 3 pipe configuration and have an eDP panel, it's > better to put the eDP output on pipe C, since then pipes A&B will have > full 4-lane fdi links to the pch ports (instead of otherwise only 2 > lanes each on links B&C) > - similarly when we have a 3 pipe configuration with all encoders on > the pch, we need to move the mode with the highest dotclock to pipe A > (since that's the only one which will have 4 lanes, pipe B&C will only > have 2 each). > - afaik these kind of asymmetric constraints won't disappear anytime > soon, haswell definitely still has some. Yeah that's a good point... adding a virtual crtc layer would solve this and let us preserve the existing ABI.
On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: > On Thu, 1 Nov 2012 12:12:35 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Send completion events when the atomic modesetting operations has > > > finished succesfully. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > I have to admit I'm not on top of the latest ioctl/interface > > discussion, but one new requirement for the modeset (not the pageflip > > part) of the all this atomic stuff I've discovered is that the kernel > > needs to be able to select the crtcs for each output completely > > unrestricted. I think userspace should simply pass in abstract crtc > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > > crtcs it has selected. > > > > We can't do that remapping internally because the crtc abstraction is leaky: > > - wait_for_vblank requires the pipe id, which could then change on every modeset > > - similarly userspace doing MI_WAIT for scanlines needs to know the > > actual hw pipe in use by a crtc. > > And current userspace presumes that the mapping between crtc->pipe > > doesn't change. > > > > An example why the kernel needs to pick the crtc for userspace: > > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to > > put the panel on the first crtc > > - but if you run in a 3 pipe configuration and have an eDP panel, it's > > better to put the eDP output on pipe C, since then pipes A&B will have > > full 4-lane fdi links to the pch ports (instead of otherwise only 2 > > lanes each on links B&C) > > - similarly when we have a 3 pipe configuration with all encoders on > > the pch, we need to move the mode with the highest dotclock to pipe A > > (since that's the only one which will have 4 lanes, pipe B&C will only > > have 2 each). > > - afaik these kind of asymmetric constraints won't disappear anytime > > soon, haswell definitely still has some. > > Yeah that's a good point... adding a virtual crtc layer would solve > this and let us preserve the existing ABI. How would the state handling work? I mean if drm_crtc X currently has some state, drm_crtc Y has some other state, and then we do a modeset which would require swapping the roles of the crtcs, what would happen to the bits of state that we didn't specify? If we'd do the remapping below the drm crtc layer, the state would always be tied to the drm crtc. But that would definitely require mostly uniform hardware "crtcs" so that the capabilities of the drm_crtcs wouldn't keep changing whenever the remap happens. Well, I suppose we could tie the state to the virtual crtc instead, and doing an operation on a real drm_crtc would also change the state of the currently bound virtual crtc. And then changing the virtual<->real mapping would just copy the state over from the virtual crtc to the real drm_crtc. And if we do it for crtcs, then we'd need to do it for planes as well, because the plane<->crtc mapping can be fixed or otherwise limited in some fashion. Either way it sounds rather messy to me. Another option would be just leave it up to userspace to make the correct choice between crtcs and planes. But then user space needs to be equipped with more hardware specific knowledge, so it's not a very appealing idea either.
On Thu, 1 Nov 2012 19:07:02 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: > > On Thu, 1 Nov 2012 12:12:35 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Send completion events when the atomic modesetting operations has > > > > finished succesfully. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > I have to admit I'm not on top of the latest ioctl/interface > > > discussion, but one new requirement for the modeset (not the pageflip > > > part) of the all this atomic stuff I've discovered is that the kernel > > > needs to be able to select the crtcs for each output completely > > > unrestricted. I think userspace should simply pass in abstract crtc > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > > > crtcs it has selected. > > > > > > We can't do that remapping internally because the crtc abstraction is leaky: > > > - wait_for_vblank requires the pipe id, which could then change on every modeset > > > - similarly userspace doing MI_WAIT for scanlines needs to know the > > > actual hw pipe in use by a crtc. > > > And current userspace presumes that the mapping between crtc->pipe > > > doesn't change. > > > > > > An example why the kernel needs to pick the crtc for userspace: > > > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to > > > put the panel on the first crtc > > > - but if you run in a 3 pipe configuration and have an eDP panel, it's > > > better to put the eDP output on pipe C, since then pipes A&B will have > > > full 4-lane fdi links to the pch ports (instead of otherwise only 2 > > > lanes each on links B&C) > > > - similarly when we have a 3 pipe configuration with all encoders on > > > the pch, we need to move the mode with the highest dotclock to pipe A > > > (since that's the only one which will have 4 lanes, pipe B&C will only > > > have 2 each). > > > - afaik these kind of asymmetric constraints won't disappear anytime > > > soon, haswell definitely still has some. > > > > Yeah that's a good point... adding a virtual crtc layer would solve > > this and let us preserve the existing ABI. > > How would the state handling work? I mean if drm_crtc X currently has > some state, drm_crtc Y has some other state, and then we do a modeset > which would require swapping the roles of the crtcs, what would happen > to the bits of state that we didn't specify? > > If we'd do the remapping below the drm crtc layer, the state would > always be tied to the drm crtc. But that would definitely require > mostly uniform hardware "crtcs" so that the capabilities of the > drm_crtcs wouldn't keep changing whenever the remap happens. > > Well, I suppose we could tie the state to the virtual crtc instead, > and doing an operation on a real drm_crtc would also change the > state of the currently bound virtual crtc. And then changing the > virtual<->real mapping would just copy the state over from the virtual > crtc to the real drm_crtc. > > And if we do it for crtcs, then we'd need to do it for planes as well, > because the plane<->crtc mapping can be fixed or otherwise limited > in some fashion. > > Either way it sounds rather messy to me. > > Another option would be just leave it up to userspace to make the > correct choice between crtcs and planes. But then user space needs > to be equipped with more hardware specific knowledge, so it's not > a very appealing idea either. Yeah it gets ugly one way or another. From a userspace perspective, keeping the ugliness in the kernel is nice, and if we have to do it somewhere I guess I'd prefer that. However, we can't always hide hw details like that in the kernel through generic interfaces (e.g. for sprites and all their restrictions) so userspace will have to have some knowledge one way or another, and maybe it's not that bad to push some of the other limitation knowledge into our userspace code. Ambivalent.
On Thu, Nov 01, 2012 at 10:12:21AM -0700, Jesse Barnes wrote: > On Thu, 1 Nov 2012 19:07:02 +0200 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: > > > On Thu, 1 Nov 2012 12:12:35 +0100 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Send completion events when the atomic modesetting operations has > > > > > finished succesfully. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > I have to admit I'm not on top of the latest ioctl/interface > > > > discussion, but one new requirement for the modeset (not the pageflip > > > > part) of the all this atomic stuff I've discovered is that the kernel > > > > needs to be able to select the crtcs for each output completely > > > > unrestricted. I think userspace should simply pass in abstract crtc > > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > > > > crtcs it has selected. > > > > > > > > We can't do that remapping internally because the crtc abstraction is leaky: > > > > - wait_for_vblank requires the pipe id, which could then change on every modeset > > > > - similarly userspace doing MI_WAIT for scanlines needs to know the > > > > actual hw pipe in use by a crtc. > > > > And current userspace presumes that the mapping between crtc->pipe > > > > doesn't change. > > > > > > > > An example why the kernel needs to pick the crtc for userspace: > > > > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to > > > > put the panel on the first crtc > > > > - but if you run in a 3 pipe configuration and have an eDP panel, it's > > > > better to put the eDP output on pipe C, since then pipes A&B will have > > > > full 4-lane fdi links to the pch ports (instead of otherwise only 2 > > > > lanes each on links B&C) > > > > - similarly when we have a 3 pipe configuration with all encoders on > > > > the pch, we need to move the mode with the highest dotclock to pipe A > > > > (since that's the only one which will have 4 lanes, pipe B&C will only > > > > have 2 each). > > > > - afaik these kind of asymmetric constraints won't disappear anytime > > > > soon, haswell definitely still has some. > > > > > > Yeah that's a good point... adding a virtual crtc layer would solve > > > this and let us preserve the existing ABI. > > > > How would the state handling work? I mean if drm_crtc X currently has > > some state, drm_crtc Y has some other state, and then we do a modeset > > which would require swapping the roles of the crtcs, what would happen > > to the bits of state that we didn't specify? > > > > If we'd do the remapping below the drm crtc layer, the state would > > always be tied to the drm crtc. But that would definitely require > > mostly uniform hardware "crtcs" so that the capabilities of the > > drm_crtcs wouldn't keep changing whenever the remap happens. > > > > Well, I suppose we could tie the state to the virtual crtc instead, > > and doing an operation on a real drm_crtc would also change the > > state of the currently bound virtual crtc. And then changing the > > virtual<->real mapping would just copy the state over from the virtual > > crtc to the real drm_crtc. > > > > And if we do it for crtcs, then we'd need to do it for planes as well, > > because the plane<->crtc mapping can be fixed or otherwise limited > > in some fashion. > > > > Either way it sounds rather messy to me. > > > > Another option would be just leave it up to userspace to make the > > correct choice between crtcs and planes. But then user space needs > > to be equipped with more hardware specific knowledge, so it's not > > a very appealing idea either. > > Yeah it gets ugly one way or another. From a userspace perspective, > keeping the ugliness in the kernel is nice, and if we have to do it > somewhere I guess I'd prefer that. My tentative Grand Plan (tm) for the atomic modeset implementation of such things is to pimp the new struct intel_crtc_config to contain all the configuration state (so with Rob's atomic modeset proposal that would also embed the drm_crtc_state struct). It would also contain all the derived state like pll settings, fdi lanes, ... Now the improve >mode_adjust stage, now called ->compute_config allocates such a struct for each crtc, does some prep, calls down into encoder->compute_config callbacks, then applies any fixups and screaming required to come up with a working global config. All rather hazy, I know ;-) But e.g. for the above case of trying to squeeze the fdi links into the available sets of fdi lanes I guess we could first compute the upper bound for the fdi link requirements (the current wip stuff already pre-computes the pipe_bpp from the fb->depth). Then check whether that fits, do any readjustments (I already have a has_pch_encoder attribute, maybe at the wrong spot still, but we should be able to know which outputs need fdi links). If there's no way to fit it, reassign pipes a bit or try dithering. Once that works, call into encoders ... Or maybe we could do a loop, since the encoders also limit the pipe_bpp. So first pipe_bpp from the fb->depth, then check for output properties (6bpc dithering on lvds, or a puny dp link), then check whether it fits into fdi. If not, dither more or reassign, then re-run the encoder config computations. If it still has too high bw requirementsmuch, give up. In any case, and disregarding the above ramblings, I plan to make the intel_crtc_config thing free-standing for the ->compute_config stage, so we could easily add a preferred_crtc pointer to it since it's not tied to a specific crtc at that point. The problem now is that the connector->encoder->crtc links are embedded into the connectors and encoders, so they are not free-standing. But if we want to support atomic modeset on all properties (which I think we should), we need to fix that anyway ... > However, we can't always hide hw details like that in the kernel > through generic interfaces (e.g. for sprites and all their > restrictions) so userspace will have to have some knowledge one way or > another, and maybe it's not that bad to push some of the other > limitation knowledge into our userspace code. I disagree, there's no sane way that userspace can figure out that the 3 pipe config it wants would work, but only if it moves the eDP output to some other pipe. So if our aim is to support that (I know, we have more pressing and less lofty things on the table), the interface should allow it. Cheers, Daniel
On Thu, Nov 01, 2012 at 11:39:58PM +0100, Daniel Vetter wrote: > On Thu, Nov 01, 2012 at 10:12:21AM -0700, Jesse Barnes wrote: > > On Thu, 1 Nov 2012 19:07:02 +0200 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: > > > > On Thu, 1 Nov 2012 12:12:35 +0100 > > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > > > Send completion events when the atomic modesetting operations has > > > > > > finished succesfully. > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > I have to admit I'm not on top of the latest ioctl/interface > > > > > discussion, but one new requirement for the modeset (not the pageflip > > > > > part) of the all this atomic stuff I've discovered is that the kernel > > > > > needs to be able to select the crtcs for each output completely > > > > > unrestricted. I think userspace should simply pass in abstract crtc > > > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > > > > > crtcs it has selected. > > > > > > > > > > We can't do that remapping internally because the crtc abstraction is leaky: > > > > > - wait_for_vblank requires the pipe id, which could then change on every modeset > > > > > - similarly userspace doing MI_WAIT for scanlines needs to know the > > > > > actual hw pipe in use by a crtc. > > > > > And current userspace presumes that the mapping between crtc->pipe > > > > > doesn't change. > > > > > > > > > > An example why the kernel needs to pick the crtc for userspace: > > > > > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to > > > > > put the panel on the first crtc > > > > > - but if you run in a 3 pipe configuration and have an eDP panel, it's > > > > > better to put the eDP output on pipe C, since then pipes A&B will have > > > > > full 4-lane fdi links to the pch ports (instead of otherwise only 2 > > > > > lanes each on links B&C) > > > > > - similarly when we have a 3 pipe configuration with all encoders on > > > > > the pch, we need to move the mode with the highest dotclock to pipe A > > > > > (since that's the only one which will have 4 lanes, pipe B&C will only > > > > > have 2 each). > > > > > - afaik these kind of asymmetric constraints won't disappear anytime > > > > > soon, haswell definitely still has some. > > > > > > > > Yeah that's a good point... adding a virtual crtc layer would solve > > > > this and let us preserve the existing ABI. > > > > > > How would the state handling work? I mean if drm_crtc X currently has > > > some state, drm_crtc Y has some other state, and then we do a modeset > > > which would require swapping the roles of the crtcs, what would happen > > > to the bits of state that we didn't specify? > > > > > > If we'd do the remapping below the drm crtc layer, the state would > > > always be tied to the drm crtc. But that would definitely require > > > mostly uniform hardware "crtcs" so that the capabilities of the > > > drm_crtcs wouldn't keep changing whenever the remap happens. > > > > > > Well, I suppose we could tie the state to the virtual crtc instead, > > > and doing an operation on a real drm_crtc would also change the > > > state of the currently bound virtual crtc. And then changing the > > > virtual<->real mapping would just copy the state over from the virtual > > > crtc to the real drm_crtc. > > > > > > And if we do it for crtcs, then we'd need to do it for planes as well, > > > because the plane<->crtc mapping can be fixed or otherwise limited > > > in some fashion. > > > > > > Either way it sounds rather messy to me. > > > > > > Another option would be just leave it up to userspace to make the > > > correct choice between crtcs and planes. But then user space needs > > > to be equipped with more hardware specific knowledge, so it's not > > > a very appealing idea either. > > > > Yeah it gets ugly one way or another. From a userspace perspective, > > keeping the ugliness in the kernel is nice, and if we have to do it > > somewhere I guess I'd prefer that. > > My tentative Grand Plan (tm) for the atomic modeset implementation of such > things is to pimp the new struct intel_crtc_config to contain all the > configuration state (so with Rob's atomic modeset proposal that would also > embed the drm_crtc_state struct). It would also contain all the derived > state like pll settings, fdi lanes, ... > > Now the improve >mode_adjust stage, now called ->compute_config allocates > such a struct for each crtc, does some prep, calls down into > encoder->compute_config callbacks, then applies any fixups and screaming > required to come up with a working global config. All rather hazy, I know > ;-) > > But e.g. for the above case of trying to squeeze the fdi links into the > available sets of fdi lanes I guess we could first compute the upper bound > for the fdi link requirements (the current wip stuff already pre-computes > the pipe_bpp from the fb->depth). Then check whether that fits, do any > readjustments (I already have a has_pch_encoder attribute, maybe at the > wrong spot still, but we should be able to know which outputs need fdi > links). If there's no way to fit it, reassign pipes a bit or try > dithering. Once that works, call into encoders ... > > Or maybe we could do a loop, since the encoders also limit the pipe_bpp. > So first pipe_bpp from the fb->depth, then check for output properties > (6bpc dithering on lvds, or a puny dp link), then check whether it fits > into fdi. If not, dither more or reassign, then re-run the encoder config > computations. If it still has too high bw requirementsmuch, give up. > > In any case, and disregarding the above ramblings, I plan to make the > intel_crtc_config thing free-standing for the ->compute_config stage, so > we could easily add a preferred_crtc pointer to it since it's not tied to > a specific crtc at that point. The problem now is that the > connector->encoder->crtc links are embedded into the connectors and > encoders, so they are not free-standing. But if we want to support atomic > modeset on all properties (which I think we should), we need to fix that > anyway ... I'm not sure what the placement of the encoder and crtc pointers has to do with atomic modeset. Now, all of what you describe above is about the internals, and seems perfectly reasonable. It sounds like the same idea had when I started plaing around with atomic modeset. That is pulling the state out of the various structures into some kind of state object that can be manipulated w/o disturbing the rest of the system. But I gave up on the idea due to the sheer scope of the work, and I wanted to make something that works sooner rather than later. My current code just goes around and shovels the new state into the object structs themselves, relying mostly on the save/restore code to reset the various bits of state to the original values when things fail. But anyway, the user visible state handling is still going to be a problem with them virtual crtcs. We need to come up with a plan for that if that's really the route we want to take.
On Thu, Nov 1, 2012 at 5:39 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Nov 01, 2012 at 10:12:21AM -0700, Jesse Barnes wrote: >> On Thu, 1 Nov 2012 19:07:02 +0200 >> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: >> > > On Thu, 1 Nov 2012 12:12:35 +0100 >> > > Daniel Vetter <daniel@ffwll.ch> wrote: >> > > >> > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: >> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > > >> > > > > Send completion events when the atomic modesetting operations has >> > > > > finished succesfully. >> > > > > >> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > >> > > > I have to admit I'm not on top of the latest ioctl/interface >> > > > discussion, but one new requirement for the modeset (not the pageflip >> > > > part) of the all this atomic stuff I've discovered is that the kernel >> > > > needs to be able to select the crtcs for each output completely >> > > > unrestricted. I think userspace should simply pass in abstract crtc >> > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual >> > > > crtcs it has selected. >> > > > >> > > > We can't do that remapping internally because the crtc abstraction is leaky: >> > > > - wait_for_vblank requires the pipe id, which could then change on every modeset >> > > > - similarly userspace doing MI_WAIT for scanlines needs to know the >> > > > actual hw pipe in use by a crtc. >> > > > And current userspace presumes that the mapping between crtc->pipe >> > > > doesn't change. I don't know if it is possible, but it would be nice to try to clean up crtc<->pipe stuff.. userspace, at least modetest, assumes crtc == crtc_list[pipe]. But I haven't noticed yet anywhere that this relationship is documented. And if you are thinking about doing what I think you are thinking about doing, then this assumption would no longer hold for i915. How frequently do you emit waits for scanline? Places where the pipe # ends up in cmdstream, would it be too crazy to handle that like a reloc where the kernel goes and fixes up the actual value in the cmdstream as it does it's GEM reloc pass? If you did this then you could virtualize pipe as well as crtc. >> > > > An example why the kernel needs to pick the crtc for userspace: >> > > > - on ivb only pipe A has a 7x5 panel fitter, so usually you want to >> > > > put the panel on the first crtc >> > > > - but if you run in a 3 pipe configuration and have an eDP panel, it's >> > > > better to put the eDP output on pipe C, since then pipes A&B will have >> > > > full 4-lane fdi links to the pch ports (instead of otherwise only 2 >> > > > lanes each on links B&C) >> > > > - similarly when we have a 3 pipe configuration with all encoders on >> > > > the pch, we need to move the mode with the highest dotclock to pipe A >> > > > (since that's the only one which will have 4 lanes, pipe B&C will only >> > > > have 2 each). >> > > > - afaik these kind of asymmetric constraints won't disappear anytime >> > > > soon, haswell definitely still has some. >> > > >> > > Yeah that's a good point... adding a virtual crtc layer would solve >> > > this and let us preserve the existing ABI. >> > >> > How would the state handling work? I mean if drm_crtc X currently has >> > some state, drm_crtc Y has some other state, and then we do a modeset >> > which would require swapping the roles of the crtcs, what would happen >> > to the bits of state that we didn't specify? >> > >> > If we'd do the remapping below the drm crtc layer, the state would >> > always be tied to the drm crtc. But that would definitely require >> > mostly uniform hardware "crtcs" so that the capabilities of the >> > drm_crtcs wouldn't keep changing whenever the remap happens. >> > >> > Well, I suppose we could tie the state to the virtual crtc instead, >> > and doing an operation on a real drm_crtc would also change the >> > state of the currently bound virtual crtc. And then changing the >> > virtual<->real mapping would just copy the state over from the virtual >> > crtc to the real drm_crtc. >> > >> > And if we do it for crtcs, then we'd need to do it for planes as well, >> > because the plane<->crtc mapping can be fixed or otherwise limited >> > in some fashion. >> > >> > Either way it sounds rather messy to me. >> > >> > Another option would be just leave it up to userspace to make the >> > correct choice between crtcs and planes. But then user space needs >> > to be equipped with more hardware specific knowledge, so it's not >> > a very appealing idea either. >> >> Yeah it gets ugly one way or another. From a userspace perspective, >> keeping the ugliness in the kernel is nice, and if we have to do it >> somewhere I guess I'd prefer that. > > My tentative Grand Plan (tm) for the atomic modeset implementation of such > things is to pimp the new struct intel_crtc_config to contain all the > configuration state (so with Rob's atomic modeset proposal that would also > embed the drm_crtc_state struct). It would also contain all the derived > state like pll settings, fdi lanes, ... > > Now the improve >mode_adjust stage, now called ->compute_config allocates > such a struct for each crtc, does some prep, calls down into > encoder->compute_config callbacks, then applies any fixups and screaming > required to come up with a working global config. All rather hazy, I know > ;-) > > But e.g. for the above case of trying to squeeze the fdi links into the > available sets of fdi lanes I guess we could first compute the upper bound > for the fdi link requirements (the current wip stuff already pre-computes > the pipe_bpp from the fb->depth). Then check whether that fits, do any > readjustments (I already have a has_pch_encoder attribute, maybe at the > wrong spot still, but we should be able to know which outputs need fdi > links). If there's no way to fit it, reassign pipes a bit or try > dithering. Once that works, call into encoders ... > > Or maybe we could do a loop, since the encoders also limit the pipe_bpp. > So first pipe_bpp from the fb->depth, then check for output properties > (6bpc dithering on lvds, or a puny dp link), then check whether it fits > into fdi. If not, dither more or reassign, then re-run the encoder config > computations. If it still has too high bw requirementsmuch, give up. > > In any case, and disregarding the above ramblings, I plan to make the > intel_crtc_config thing free-standing for the ->compute_config stage, so > we could easily add a preferred_crtc pointer to it since it's not tied to > a specific crtc at that point. The problem now is that the > connector->encoder->crtc links are embedded into the connectors and > encoders, so they are not free-standing. But if we want to support atomic > modeset on all properties (which I think we should), we need to fix that > anyway ... I think for full atomic modeset, then the drm_*_state structs end up having the link pointers, ie 'struct drm_connector_state' has the ptr to 'struct drm_encoder', and so on.. BR, -R > >> However, we can't always hide hw details like that in the kernel >> through generic interfaces (e.g. for sprites and all their >> restrictions) so userspace will have to have some knowledge one way or >> another, and maybe it's not that bad to push some of the other >> limitation knowledge into our userspace code. > > I disagree, there's no sane way that userspace can figure out that the 3 > pipe config it wants would work, but only if it moves the eDP output to > some other pipe. So if our aim is to support that (I know, we have more > pressing and less lofty things on the table), the interface should allow > it. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Nov 07, 2012 at 02:29:45PM -0600, Rob Clark wrote: > On Thu, Nov 1, 2012 at 5:39 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Nov 01, 2012 at 10:12:21AM -0700, Jesse Barnes wrote: > >> On Thu, 1 Nov 2012 19:07:02 +0200 > >> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > >> > >> > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: > >> > > On Thu, 1 Nov 2012 12:12:35 +0100 > >> > > Daniel Vetter <daniel@ffwll.ch> wrote: > >> > > > >> > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: > >> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > > > > >> > > > > Send completion events when the atomic modesetting operations has > >> > > > > finished succesfully. > >> > > > > > >> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > > > >> > > > I have to admit I'm not on top of the latest ioctl/interface > >> > > > discussion, but one new requirement for the modeset (not the pageflip > >> > > > part) of the all this atomic stuff I've discovered is that the kernel > >> > > > needs to be able to select the crtcs for each output completely > >> > > > unrestricted. I think userspace should simply pass in abstract crtc > >> > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual > >> > > > crtcs it has selected. > >> > > > > >> > > > We can't do that remapping internally because the crtc abstraction is leaky: > >> > > > - wait_for_vblank requires the pipe id, which could then change on every modeset > >> > > > - similarly userspace doing MI_WAIT for scanlines needs to know the > >> > > > actual hw pipe in use by a crtc. > >> > > > And current userspace presumes that the mapping between crtc->pipe > >> > > > doesn't change. > > I don't know if it is possible, but it would be nice to try to clean > up crtc<->pipe stuff.. userspace, at least modetest, assumes crtc == > crtc_list[pipe]. But I haven't noticed yet anywhere that this > relationship is documented. And if you are thinking about doing what > I think you are thinking about doing, then this assumption would no > longer hold for i915. This relationship does already no longer hold on i915 - on gen3 at least we sometimes switch the crtc->pipe assignement (to make fbc more useful), which means even with todays code userspace cannot assume (when running on i915), that the vblank pipe satisfies crtc == crtc_list[pipe]. > How frequently do you emit waits for scanline? Places where the pipe > # ends up in cmdstream, would it be too crazy to handle that like a > reloc where the kernel goes and fixes up the actual value in the > cmdstream as it does it's GEM reloc pass? If you did this then you > could virtualize pipe as well as crtc. Every dri2 copyregion or Xv upload to the frontbuffer on pre-gen6 will use it. And we need old userspace to keep on working - presumably the fb layer will switch to using the new atomic modeset stuff (if available) to figure out an optimal config, so this is relevant. -Daniel
On Fri, Nov 9, 2012 at 3:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Nov 07, 2012 at 02:29:45PM -0600, Rob Clark wrote: >> On Thu, Nov 1, 2012 at 5:39 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Thu, Nov 01, 2012 at 10:12:21AM -0700, Jesse Barnes wrote: >> >> On Thu, 1 Nov 2012 19:07:02 +0200 >> >> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> >> >> > On Thu, Nov 01, 2012 at 07:39:12AM -0700, Jesse Barnes wrote: >> >> > > On Thu, 1 Nov 2012 12:12:35 +0100 >> >> > > Daniel Vetter <daniel@ffwll.ch> wrote: >> >> > > >> >> > > > On Thu, Oct 25, 2012 at 8:05 PM, <ville.syrjala@linux.intel.com> wrote: >> >> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> > > > > >> >> > > > > Send completion events when the atomic modesetting operations has >> >> > > > > finished succesfully. >> >> > > > > >> >> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> > > > >> >> > > > I have to admit I'm not on top of the latest ioctl/interface >> >> > > > discussion, but one new requirement for the modeset (not the pageflip >> >> > > > part) of the all this atomic stuff I've discovered is that the kernel >> >> > > > needs to be able to select the crtcs for each output completely >> >> > > > unrestricted. I think userspace should simply pass in abstract crtc >> >> > > > ids (e.g. 0, 1, 2, ...) and the kernel then passes back the actual >> >> > > > crtcs it has selected. >> >> > > > >> >> > > > We can't do that remapping internally because the crtc abstraction is leaky: >> >> > > > - wait_for_vblank requires the pipe id, which could then change on every modeset >> >> > > > - similarly userspace doing MI_WAIT for scanlines needs to know the >> >> > > > actual hw pipe in use by a crtc. >> >> > > > And current userspace presumes that the mapping between crtc->pipe >> >> > > > doesn't change. >> >> I don't know if it is possible, but it would be nice to try to clean >> up crtc<->pipe stuff.. userspace, at least modetest, assumes crtc == >> crtc_list[pipe]. But I haven't noticed yet anywhere that this >> relationship is documented. And if you are thinking about doing what >> I think you are thinking about doing, then this assumption would no >> longer hold for i915. > > This relationship does already no longer hold on i915 - on gen3 at least > we sometimes switch the crtc->pipe assignement (to make fbc more useful), > which means even with todays code userspace cannot assume (when running on > i915), that the vblank pipe satisfies crtc == crtc_list[pipe]. hmm, how does this not break weston compositor-drm (and modetest w/ vsync'd flipping.. although I suppose that is a less important use-case) >> How frequently do you emit waits for scanline? Places where the pipe >> # ends up in cmdstream, would it be too crazy to handle that like a >> reloc where the kernel goes and fixes up the actual value in the >> cmdstream as it does it's GEM reloc pass? If you did this then you >> could virtualize pipe as well as crtc. > > Every dri2 copyregion or Xv upload to the frontbuffer on pre-gen6 will use > it. And we need old userspace to keep on working - presumably the fb layer > will switch to using the new atomic modeset stuff (if available) to figure > out an optimal config, so this is relevant. yeah, not quite sure how the backwards-compat should work.. you'd have to somehow only dynamically reassign crtcs if you could tell that userspace is new enough to cope.. BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 4899f8c..3adb140 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -53,6 +53,7 @@ struct intel_plane_state { bool dirty; bool pinned; bool changed; + struct drm_pending_atomic_event *event; struct { struct drm_crtc *crtc; @@ -74,6 +75,7 @@ struct intel_crtc_state { unsigned long connectors_bitmask; unsigned long encoders_bitmask; bool changed; + struct drm_pending_atomic_event *event; struct { bool enabled; @@ -922,6 +924,111 @@ int intel_commit_plane_nopin(struct drm_plane *plane, struct drm_framebuffer *fb, const struct intel_plane_coords *coords); +static struct drm_pending_atomic_event *alloc_event(struct drm_device *dev, + struct drm_file *file_priv, + uint64_t user_data) +{ + struct drm_pending_atomic_event *e; + unsigned long flags; + + spin_lock_irqsave(&dev->event_lock, flags); + + if (file_priv->event_space < sizeof e->event) { + spin_unlock_irqrestore(&dev->event_lock, flags); + return ERR_PTR(-ENOSPC); + } + + file_priv->event_space -= sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + + e = kzalloc(sizeof *e, GFP_KERNEL); + if (!e) { + spin_lock_irqsave(&dev->event_lock, flags); + file_priv->event_space += sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + + return ERR_PTR(-ENOMEM); + } + + e->event.base.type = DRM_EVENT_ATOMIC_COMPLETE; + e->event.base.length = sizeof e->event; + e->event.user_data = user_data; + e->base.event = &e->event.base; + e->base.file_priv = file_priv; + e->base.destroy = (void (*) (struct drm_pending_event *)) kfree; + + return e; +} + +static void free_event(struct drm_pending_atomic_event *e) +{ + e->base.file_priv->event_space += sizeof e->event; + kfree(e); +} + +static void queue_event(struct drm_device *dev, struct drm_crtc *crtc, + struct drm_pending_atomic_event *e) +{ + struct timeval tvbl; + + if (crtc) { + int pipe = to_intel_crtc(crtc)->pipe; + + /* FIXME this is wrong for flips that are completed not at vblank */ + e->event.sequence = drm_vblank_count_and_time(dev, pipe, &tvbl); + e->event.tv_sec = tvbl.tv_sec; + e->event.tv_usec = tvbl.tv_usec; + } else { + e->event.sequence = 0; + e->event.tv_sec = 0; + e->event.tv_usec = 0; + } + + list_add_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); +} + +static void queue_remaining_events(struct drm_device *dev, struct intel_atomic_state *s) +{ + int i; + + for (i = 0; i < dev->mode_config.num_crtc; i++) { + struct intel_crtc_state *st = &s->crtc[i]; + + if (st->event) { + if (st->old.fb) + st->event->event.old_fb_id = st->old.fb->base.id; + + spin_lock_irq(&dev->event_lock); + queue_event(dev, st->crtc, st->event); + spin_unlock_irq(&dev->event_lock); + + st->event = NULL; + } + } + + for (i = 0; i < dev->mode_config.num_plane; i++) { + struct intel_plane_state *st = &s->plane[i]; + struct drm_crtc *crtc; + + if (!st->event) + continue; + + crtc = st->plane->crtc; + if (!crtc) + crtc = st->old.crtc; + + if (st->old.fb) + st->event->event.old_fb_id = st->old.fb->base.id; + + spin_lock_irq(&dev->event_lock); + queue_event(dev, crtc, st->event); + spin_unlock_irq(&dev->event_lock); + + st->event = NULL; + } +} + static void swap_old_new(struct drm_device *dev, struct intel_atomic_state *s) { @@ -1426,6 +1533,73 @@ static void update_crtc(struct drm_device *dev, } } +static int alloc_flip_data(struct drm_device *dev, struct intel_atomic_state *s) +{ + int i; + + for (i = 0; i < dev->mode_config.num_crtc; i++) { + struct intel_crtc_state *st = &s->crtc[i]; + + if (st->changed && s->flags & DRM_MODE_ATOMIC_EVENT) { + struct drm_pending_atomic_event *e; + + e = alloc_event(dev, s->file, s->user_data); + if (IS_ERR(e)) + return PTR_ERR(e); + + e->event.obj_id = st->crtc->base.id; + + st->event = e; + } + } + + + for (i = 0; i < dev->mode_config.num_plane; i++) { + struct intel_plane_state *st = &s->plane[i]; + + if (st->changed && s->flags & DRM_MODE_ATOMIC_EVENT) { + struct drm_pending_atomic_event *e; + + e = alloc_event(dev, s->file, s->user_data); + if (IS_ERR(e)) + return PTR_ERR(e); + + e->event.obj_id = st->plane->base.id; + + st->event = e; + } + } + + return 0; +} + +static void free_flip_data(struct drm_device *dev, struct intel_atomic_state *s) +{ + int i; + + for (i = 0; i < dev->mode_config.num_crtc; i++) { + struct intel_crtc_state *st = &s->crtc[i]; + + if (st->event) { + spin_lock_irq(&dev->event_lock); + free_event(st->event); + spin_unlock_irq(&dev->event_lock); + st->event = NULL; + } + } + + for (i = 0; i < dev->mode_config.num_plane; i++) { + struct intel_plane_state *st = &s->plane[i]; + + if (st->event) { + spin_lock_irq(&dev->event_lock); + free_event(st->event); + spin_unlock_irq(&dev->event_lock); + st->event = NULL; + } + } +} + static int intel_atomic_commit(struct drm_device *dev, void *state) { struct intel_atomic_state *s = state; @@ -1434,12 +1608,13 @@ static int intel_atomic_commit(struct drm_device *dev, void *state) if (s->flags & DRM_MODE_ATOMIC_NONBLOCK) return -ENOSYS; - if (s->flags & DRM_MODE_ATOMIC_EVENT) - return -ENOSYS; - if (!s->dirty) return 0; + ret = alloc_flip_data(dev, s); + if (ret) + return ret; + ret = pin_fbs(dev, s); if (ret) return ret; @@ -1460,6 +1635,17 @@ static int intel_atomic_commit(struct drm_device *dev, void *state) unpin_old_cursors(dev, s); unpin_old_fbs(dev, s); + /* + * Either we took the blocking code path, or perhaps the state of + * some objects didn't actually change? Nonetheless the user wanted + * events for all objects he touched, so queue up any events that + * are still pending. + * + * FIXME this needs more work. If the previous flip is still pending + * we shouldn't send this event until that flip completes. + */ + queue_remaining_events(dev, s); + update_plane_obj(dev, s); update_crtc(dev, s); @@ -1473,6 +1659,9 @@ static void intel_atomic_end(struct drm_device *dev, void *state) { struct intel_atomic_state *s = state; + /* don't send events when restoring old state */ + free_flip_data(dev, state); + /* restore the state of all objects */ if (s->restore_state) restore_state(dev, state);