| Message ID | 20161118233733.GP1041@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote: > Hi, > > While testing HDMI with Xorg on the Juno board, I find that when Xorg > starts up or shuts down, the display is shifted significantly to the > right and wrapped in the active region. (No sync bars are visible.) > The timings are correct, it behaves as if the start address has been > shifted many pixels _into_ the framebuffer. > > This occurs whenever the display mode size is changed - using xrandr > in Xorg shows that changing the resolution triggers the problem > almost every time, but changing the refresh rate does not. > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > and repeated disable/enable cycles do not make the issue re-appear. > > So, I patched the HDLCD to do this, and testing it with Xorg after > several repetitions seems to work. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > What I think is going on is that the FIFO or address generator for > reading data from the AXI bus is not properly reset when changing the > resolution, and the enable-disable-enable cycle causes the HDLCD > hardware to sort itself out. It's (eg) significantly out - for example, > to properly align the display, I have to program an address of > 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels > before the real start of the frame buffer. > > With this patch, a patch to TDA998x to avoid the i2c-designware issue, > and xf86-video-armada, I have LXDE running on the Juno. > > Something I also noticed is this: > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > plane->state->crtc_y * plane->state->fb->pitches[0] + > plane->state->crtc_x * bpp / 8; > > Surely this should be using src_[xy] (which are the position in the > source - iow, memory, and not crtc_[xy] which is the position on the > CRTC displayed window. To put it another way, the src_* define the > region of the source material that is mapped onto a rectangular area > on the display defined by crtc_*. > > Another note is that since the CRTC can't place the plane in arbitary > positions and sizes within the active area, should the atomic_check > ensure that crtc_x = crtc_y = 0, and the crtc width/height are the > size of the active area? Yup, it should. See drm_plane_helper_check_state() and its caller for a helper to make this easier. Long-term computing this stuff by default and having a bunch of igts to regression-test it would be good I think, but that needs CRC support. And lots of work, since we have lots of drivers. -Daniel > > drivers/gpu/drm/arm/hdlcd_crtc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > index 48019ae22ddb..3e97acf6e2a7 100644 > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc) > clk_prepare_enable(hdlcd->clk); > hdlcd_crtc_mode_set_nofb(crtc); > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > } > > static void hdlcd_crtc_disable(struct drm_crtc *crtc) > > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote: > Hi, Hi Russell, > > While testing HDMI with Xorg on the Juno board, I find that when Xorg > starts up or shuts down, the display is shifted significantly to the > right and wrapped in the active region. (No sync bars are visible.) > The timings are correct, it behaves as if the start address has been > shifted many pixels _into_ the framebuffer. > > This occurs whenever the display mode size is changed - using xrandr > in Xorg shows that changing the resolution triggers the problem > almost every time, but changing the refresh rate does not. Thanks for reporting this. To double check your issue, you are booting with HDLCD using the native monitor resolution as detected via EDID and then using xrandr to change the display mode. When you do that you are seeing the image being shifted to the right. Is that a correct description? (I'm trying to reproduce it here and want to make sure I've got the details right). > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > and repeated disable/enable cycles do not make the issue re-appear. Do you resize the display mode as well afer re-enabling HDLCD? > > So, I patched the HDLCD to do this, and testing it with Xorg after > several repetitions seems to work. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > What I think is going on is that the FIFO or address generator for > reading data from the AXI bus is not properly reset when changing the > resolution, and the enable-disable-enable cycle causes the HDLCD > hardware to sort itself out. That is likely what is happening. According to the datasheet, changing the resolution should be done while the HDLCD command mode is disabled, which is what writing 0 into HDLCD_REG_COMMAND does. > It's (eg) significantly out - for example, > to properly align the display, I have to program an address of > 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels > before the real start of the frame buffer. What is the resolution you are using? > > With this patch, a patch to TDA998x to avoid the i2c-designware issue, > and xf86-video-armada, I have LXDE running on the Juno. Can you tell me more about the TDA998x and i2c-designware issue? Also, I don't think you need to use xf86-video-armada, the mode-setting driver built into Xorg should be working fine (that is what I've used in my testing). > > Something I also noticed is this: > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > plane->state->crtc_y * plane->state->fb->pitches[0] + > plane->state->crtc_x * bpp / 8; > > Surely this should be using src_[xy] (which are the position in the > source - iow, memory, and not crtc_[xy] which is the position on the > CRTC displayed window. To put it another way, the src_* define the > region of the source material that is mapped onto a rectangular area > on the display defined by crtc_*. Yes, that is a bug and most likely the source of the issue that you are seeing if my understanding of your testing is correct. > > Another note is that since the CRTC can't place the plane in arbitary > positions and sizes within the active area, should the atomic_check > ensure that crtc_x = crtc_y = 0, and the crtc width/height are the > size of the active area? That should be the case, indeed. I'm going prepare a patch to do that. > > drivers/gpu/drm/arm/hdlcd_crtc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > index 48019ae22ddb..3e97acf6e2a7 100644 > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc) > clk_prepare_enable(hdlcd->clk); > hdlcd_crtc_mode_set_nofb(crtc); > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); I am not convinced that this is the right fix. If anything, I would put a hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); line before hdlcd_crtc_mode_set_nofs(crtc); line to make sure the command mode is disabled before setting the mode, but again, I need to understand your use case to make sure that this indeed fixes it. Best regards, Liviu > } > > static void hdlcd_crtc_disable(struct drm_crtc *crtc) > > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, Nov 21, 2016 at 11:06:04AM +0000, Liviu Dudau wrote: > On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote: > > Hi, > > Hi Russell, > > > > > While testing HDMI with Xorg on the Juno board, I find that when Xorg > > starts up or shuts down, the display is shifted significantly to the > > right and wrapped in the active region. (No sync bars are visible.) > > The timings are correct, it behaves as if the start address has been > > shifted many pixels _into_ the framebuffer. > > > > This occurs whenever the display mode size is changed - using xrandr > > in Xorg shows that changing the resolution triggers the problem > > almost every time, but changing the refresh rate does not. > > Thanks for reporting this. To double check your issue, you are booting > with HDLCD using the native monitor resolution as detected via EDID > and then using xrandr to change the display mode. When you do that you > are seeing the image being shifted to the right. Is that a correct > description? (I'm trying to reproduce it here and want to make sure > I've got the details right). I first noticed it when booting with the buggy I2C EDID reading, so DRM wasn't seeing a valid EDID. Then when Xorg started up and shut down, I noticed that the framebuffer console was shifted. It's actually shifted to the left because framebuffer pixel 0,0 is not displayed. > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > > and repeated disable/enable cycles do not make the issue re-appear. > > Do you resize the display mode as well afer re-enabling HDLCD? I quite literally just did: ./devmem2 0x7ff60230 w 0; ./devmem2 0x7ff60230 w 1 (with a devmem2 fixed for ARM64) which immediately fixed the issue. > > What I think is going on is that the FIFO or address generator for > > reading data from the AXI bus is not properly reset when changing the > > resolution, and the enable-disable-enable cycle causes the HDLCD > > hardware to sort itself out. > > That is likely what is happening. According to the datasheet, changing > the resolution should be done while the HDLCD command mode is disabled, > which is what writing 0 into HDLCD_REG_COMMAND does. That does not appear to be sufficient. > > It's (eg) significantly out - for example, > > to properly align the display, I have to program an address of > > 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels > > before the real start of the frame buffer. > > What is the resolution you are using? In the case I detailed here, 1920x1080. > > With this patch, a patch to TDA998x to avoid the i2c-designware issue, > > and xf86-video-armada, I have LXDE running on the Juno. > > Can you tell me more about the TDA998x and i2c-designware issue? > Also, I don't think you need to use xf86-video-armada, the mode-setting > driver built into Xorg should be working fine (that is what I've used > in my testing). See the i2c-designware thread on lakml. It's a spontaneous high interrupt latency causing the Tx FIFO not to be loaded before it empties, and the i2c-designware crap decides at that point to immediately generate an I2C stop. The I2C controller in Juno can only work reliably in a system which has guaranteed low interrupt latencies. > > Something I also noticed is this: > > > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > > plane->state->crtc_y * plane->state->fb->pitches[0] + > > plane->state->crtc_x * bpp / 8; > > > > Surely this should be using src_[xy] (which are the position in the > > source - iow, memory, and not crtc_[xy] which is the position on the > > CRTC displayed window. To put it another way, the src_* define the > > region of the source material that is mapped onto a rectangular area > > on the display defined by crtc_*. > > Yes, that is a bug and most likely the source of the issue that you are > seeing if my understanding of your testing is correct. It isn't the source of this issue at all. gem->paddr is 0xf5000000, and the value programmed originally into the register is the same. So, from those two pieces of information, we can reasonably assume that crtc_y and crtc_x were both zero here. > > Another note is that since the CRTC can't place the plane in arbitary > > positions and sizes within the active area, should the atomic_check > > ensure that crtc_x = crtc_y = 0, and the crtc width/height are the > > size of the active area? > > That should be the case, indeed. I'm going prepare a patch to do that. I've already a patch along the lines of Daniel Vetter's response to this point which I'm just testing. > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > > index 48019ae22ddb..3e97acf6e2a7 100644 > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > > @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc) > > clk_prepare_enable(hdlcd->clk); > > hdlcd_crtc_mode_set_nofb(crtc); > > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); > > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > > I am not convinced that this is the right fix. If anything, I would put a > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); line before hdlcd_crtc_mode_set_nofs(crtc); > line to make sure the command mode is disabled before setting the mode, but > again, I need to understand your use case to make sure that this indeed fixes it. Maybe hdlcd shouldn't be implementing the ->enable callback but instead the ->commit callback then? I'll give it a try.
On Mon, Nov 21, 2016 at 11:20:30AM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 11:06:04AM +0000, Liviu Dudau wrote: > > On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote: > > > Hi, > > > > Hi Russell, > > > > > > > > While testing HDMI with Xorg on the Juno board, I find that when Xorg > > > starts up or shuts down, the display is shifted significantly to the > > > right and wrapped in the active region. (No sync bars are visible.) > > > The timings are correct, it behaves as if the start address has been > > > shifted many pixels _into_ the framebuffer. > > > > > > This occurs whenever the display mode size is changed - using xrandr > > > in Xorg shows that changing the resolution triggers the problem > > > almost every time, but changing the refresh rate does not. > > > > Thanks for reporting this. To double check your issue, you are booting > > with HDLCD using the native monitor resolution as detected via EDID > > and then using xrandr to change the display mode. When you do that you > > are seeing the image being shifted to the right. Is that a correct > > description? (I'm trying to reproduce it here and want to make sure > > I've got the details right). > > I first noticed it when booting with the buggy I2C EDID reading, so > DRM wasn't seeing a valid EDID. Then when Xorg started up and shut > down, I noticed that the framebuffer console was shifted. It's actually > shifted to the left because framebuffer pixel 0,0 is not displayed. I see. So the reason why I did not notice this was the EDID transfers mostly working for me. > > > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > > > and repeated disable/enable cycles do not make the issue re-appear. > > > > Do you resize the display mode as well afer re-enabling HDLCD? > > I quite literally just did: > > ./devmem2 0x7ff60230 w 0; ./devmem2 0x7ff60230 w 1 Sorry, was not very clear. Under my assumption that you were resizing the display with xrandr, I was wondering if the issue you were seeing disappeared when using devmem2 plus the resizing. > > (with a devmem2 fixed for ARM64) which immediately fixed the issue. > > > > What I think is going on is that the FIFO or address generator for > > > reading data from the AXI bus is not properly reset when changing the > > > resolution, and the enable-disable-enable cycle causes the HDLCD > > > hardware to sort itself out. > > > > That is likely what is happening. According to the datasheet, changing > > the resolution should be done while the HDLCD command mode is disabled, > > which is what writing 0 into HDLCD_REG_COMMAND does. > > That does not appear to be sufficient. > > > > It's (eg) significantly out - for example, > > > to properly align the display, I have to program an address of > > > 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels > > > before the real start of the frame buffer. > > > > What is the resolution you are using? > > In the case I detailed here, 1920x1080. > > > > With this patch, a patch to TDA998x to avoid the i2c-designware issue, > > > and xf86-video-armada, I have LXDE running on the Juno. > > > > Can you tell me more about the TDA998x and i2c-designware issue? > > Also, I don't think you need to use xf86-video-armada, the mode-setting > > driver built into Xorg should be working fine (that is what I've used > > in my testing). > > See the i2c-designware thread on lakml. It's a spontaneous high > interrupt latency causing the Tx FIFO not to be loaded before it > empties, and the i2c-designware crap decides at that point to > immediately generate an I2C stop. The I2C controller in Juno can > only work reliably in a system which has guaranteed low interrupt > latencies. Sorry, my email setup had a hickup and it was slow fetching all my emails. I've seen the thread after replying in this thread. > > > > Something I also noticed is this: > > > > > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > > > plane->state->crtc_y * plane->state->fb->pitches[0] + > > > plane->state->crtc_x * bpp / 8; > > > > > > Surely this should be using src_[xy] (which are the position in the > > > source - iow, memory, and not crtc_[xy] which is the position on the > > > CRTC displayed window. To put it another way, the src_* define the > > > region of the source material that is mapped onto a rectangular area > > > on the display defined by crtc_*. > > > > Yes, that is a bug and most likely the source of the issue that you are > > seeing if my understanding of your testing is correct. > > It isn't the source of this issue at all. gem->paddr is 0xf5000000, and > the value programmed originally into the register is the same. So, from > those two pieces of information, we can reasonably assume that crtc_y > and crtc_x were both zero here. Yes, they should be zero all the time, as we don't support plane positioning with HDLCD. > > > > Another note is that since the CRTC can't place the plane in arbitary > > > positions and sizes within the active area, should the atomic_check > > > ensure that crtc_x = crtc_y = 0, and the crtc width/height are the > > > size of the active area? > > > > That should be the case, indeed. I'm going prepare a patch to do that. > > I've already a patch along the lines of Daniel Vetter's response to this > point which I'm just testing. OK, let me know how it goes and please Cc me on it. > > > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > > > index 48019ae22ddb..3e97acf6e2a7 100644 > > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > > > @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc) > > > clk_prepare_enable(hdlcd->clk); > > > hdlcd_crtc_mode_set_nofb(crtc); > > > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > > > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); > > > + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > > > > I am not convinced that this is the right fix. If anything, I would put a > > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); line before hdlcd_crtc_mode_set_nofs(crtc); > > line to make sure the command mode is disabled before setting the mode, but > > again, I need to understand your use case to make sure that this indeed fixes it. > > Maybe hdlcd shouldn't be implementing the ->enable callback but instead > the ->commit callback then? I believe we need ->enable for the initial setup (cold boot or module reloading). Best regards, Liviu > > I'll give it a try. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, Nov 21, 2016 at 11:32:12AM +0000, Liviu Dudau wrote: > On Mon, Nov 21, 2016 at 11:20:30AM +0000, Russell King - ARM Linux wrote: > > I first noticed it when booting with the buggy I2C EDID reading, so > > DRM wasn't seeing a valid EDID. Then when Xorg started up and shut > > down, I noticed that the framebuffer console was shifted. It's actually > > shifted to the left because framebuffer pixel 0,0 is not displayed. > > I see. So the reason why I did not notice this was the EDID transfers > mostly working for me. It also happens when EDID transfers work too! > > > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > > > > and repeated disable/enable cycles do not make the issue re-appear. > > > > > > Do you resize the display mode as well afer re-enabling HDLCD? > > > > I quite literally just did: > > > > ./devmem2 0x7ff60230 w 0; ./devmem2 0x7ff60230 w 1 > > Sorry, was not very clear. Under my assumption that you were resizing the > display with xrandr, I was wondering if the issue you were seeing disappeared > when using devmem2 plus the resizing. I think the problems are much deeper. I've added this: static void hdlcd_crtc_enable(struct drm_crtc *crtc) { struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); printk("%s: active %d cmd %08x\n", __func__, crtc->state->active, hdlcd_read(hdlcd, HDLCD_REG_COMMAND)); clk_prepare_enable(hdlcd->clk); ... static void hdlcd_crtc_disable(struct drm_crtc *crtc) { struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); printk("%s: active %d\n", __func__, crtc->state->active); if (!crtc->state->active) return; What I see in the kernel log each time I change the resolution is: [ 221.409577] hdlcd_crtc_disable: active 0 [ 221.430206] hdlcd_crtc_enable: active 1 cmd 00000001 [ 239.264672] hdlcd_crtc_disable: active 0 [ 239.285180] hdlcd_crtc_enable: active 1 cmd 00000001 [ 278.712792] hdlcd_crtc_disable: active 0 [ 278.730361] hdlcd_crtc_enable: active 1 cmd 00000001 [ 281.633841] hdlcd_crtc_disable: active 0 [ 281.668578] hdlcd_crtc_enable: active 1 cmd 00000001 So, when ->disable is called, active is always zero. This leads to... $ head -n3 /sys/kernel/debug/clk/clk_summary clock enable_cnt prepare_cnt rate accuracy phase ---------------------------------------------------------------------------------------- pxlclk 6 6 148500000 0 0 the enable and prepare counts for this clock incrementing by one each time I change the resolution. > > Maybe hdlcd shouldn't be implementing the ->enable callback but instead > > the ->commit callback then? > > I believe we need ->enable for the initial setup (cold boot or module > reloading). Yes, I found a comment in DRM saying that ->commit is for legacy drivers only. I think the problem is that hdlcd is not really knowing what the true state of the CRTC is, as illustrated by the clock counts increasing and the state of crtc->state->active. I'm wondering if this is a core DRM bug though... the comments and code do not align: /** * drm_atomic_helper_commit_tail - commit atomic update to hardware * @state: new modeset state to be committed void drm_atomic_helper_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; drm_atomic_helper_commit_modeset_disables(dev, state); /** * drm_atomic_helper_commit_modeset_disables - modeset commit to disable outputs * @dev: DRM device * @old_state: atomic state object with old state structures void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev, struct drm_atomic_state *old_state) So, is "state" in drm_atomic_helper_commit_tail the old state or the new state? Should this state be passed to drm_atomic_helper_commit_modeset_disables(), which seems to expect the old state? It looks _really_ screwed up here - in any case, it really doesn't help when you're not experienced with atomic mode set to work out what the hell this code is doing... it seems to be a horrible mess. Maybe someone who understands this code ought to read through it from the point of view of someone who doesn't understand it and fix the comments, or get rid of the down-right misleading comments. Comments are worse than useless if they mislead. Better to have no comments than misleading comments. Daniel?
On Mon, Nov 21, 2016 at 12:25:56PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 11:32:12AM +0000, Liviu Dudau wrote: > > On Mon, Nov 21, 2016 at 11:20:30AM +0000, Russell King - ARM Linux wrote: > > > I first noticed it when booting with the buggy I2C EDID reading, so > > > DRM wasn't seeing a valid EDID. Then when Xorg started up and shut > > > down, I noticed that the framebuffer console was shifted. It's actually > > > shifted to the left because framebuffer pixel 0,0 is not displayed. > > > > I see. So the reason why I did not notice this was the EDID transfers > > mostly working for me. > > It also happens when EDID transfers work too! > > > > > > Using devmem2 to disable and re-enable the HDLCD resolves the issue, > > > > > and repeated disable/enable cycles do not make the issue re-appear. > > > > > > > > Do you resize the display mode as well afer re-enabling HDLCD? > > > > > > I quite literally just did: > > > > > > ./devmem2 0x7ff60230 w 0; ./devmem2 0x7ff60230 w 1 > > > > Sorry, was not very clear. Under my assumption that you were resizing the > > display with xrandr, I was wondering if the issue you were seeing disappeared > > when using devmem2 plus the resizing. > > I think the problems are much deeper. I've added this: > > static void hdlcd_crtc_enable(struct drm_crtc *crtc) > { > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > printk("%s: active %d cmd %08x\n", __func__, crtc->state->active, hdlcd_read(hdlcd, HDLCD_REG_COMMAND)); > clk_prepare_enable(hdlcd->clk); > > ... > static void hdlcd_crtc_disable(struct drm_crtc *crtc) > { > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > printk("%s: active %d\n", __func__, crtc->state->active); > if (!crtc->state->active) > return; > > What I see in the kernel log each time I change the resolution is: > > [ 221.409577] hdlcd_crtc_disable: active 0 > [ 221.430206] hdlcd_crtc_enable: active 1 cmd 00000001 > [ 239.264672] hdlcd_crtc_disable: active 0 > [ 239.285180] hdlcd_crtc_enable: active 1 cmd 00000001 > [ 278.712792] hdlcd_crtc_disable: active 0 > [ 278.730361] hdlcd_crtc_enable: active 1 cmd 00000001 > [ 281.633841] hdlcd_crtc_disable: active 0 > [ 281.668578] hdlcd_crtc_enable: active 1 cmd 00000001 > > So, when ->disable is called, active is always zero. That is expected, the DRM framework will determine that the crtc is no longer active and call ->disable hook on the CRTC helper struct. > This leads to... > > $ head -n3 /sys/kernel/debug/clk/clk_summary > clock enable_cnt prepare_cnt rate accuracy > phase > ---------------------------------------------------------------------------------------- > pxlclk 6 6 148500000 0 0 > > the enable and prepare counts for this clock incrementing by one each > time I change the resolution. That is mostly due to the check in hdlcd_crtc_disable() which I should remove, I've added it because I was getting a ->disable() hook call before any ->enable() was called at startup time. I need to revisit this as I remember Daniel was commenting that this was not needed. > > > > Maybe hdlcd shouldn't be implementing the ->enable callback but instead > > > the ->commit callback then? > > > > I believe we need ->enable for the initial setup (cold boot or module > > reloading). > > Yes, I found a comment in DRM saying that ->commit is for legacy drivers > only. > > I think the problem is that hdlcd is not really knowing what the true > state of the CRTC is, as illustrated by the clock counts increasing > and the state of crtc->state->active. I think crtc->state->active is correct, we are just not acting as we should in HDLCD. > > I'm wondering if this is a core DRM bug though... the comments and > code do not align: > > /** > * drm_atomic_helper_commit_tail - commit atomic update to hardware > * @state: new modeset state to be committed > > void drm_atomic_helper_commit_tail(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > > drm_atomic_helper_commit_modeset_disables(dev, state); > > /** > * drm_atomic_helper_commit_modeset_disables - modeset commit to disable outputs * @dev: DRM device > * @old_state: atomic state object with old state structures > void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev, > struct drm_atomic_state *old_state) > > So, is "state" in drm_atomic_helper_commit_tail the old state or the > new state? Should this state be passed to > drm_atomic_helper_commit_modeset_disables(), which seems to expect > the old state? Yes, you have reached one (of the many?) DRM quirks. When drm_atomic_helper_commit_tail() gets called the *state pointer contains the old state that was swapped out by drm_atomic_helper_commit() function before calling commit_tail(). The comment on drm_atomic_helper_commit_tail function (and maybe parameter name) should be updated. > > It looks _really_ screwed up here - in any case, it really doesn't > help when you're not experienced with atomic mode set to work out > what the hell this code is doing... it seems to be a horrible mess. > Maybe someone who understands this code ought to read through it > from the point of view of someone who doesn't understand it and fix > the comments, or get rid of the down-right misleading comments. > > Comments are worse than useless if they mislead. Better to have no > comments than misleading comments. Sometimes code gets shuffled around and functions that made sense in one workflow are kept in the new workflow except their order gets changed. I believe that was the case for the function above. Best regards, Liviu > > Daniel? > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > That is mostly due to the check in hdlcd_crtc_disable() which I should > remove, I've added it because I was getting a ->disable() hook call > before any ->enable() was called at startup time. I need to revisit > this as I remember Daniel was commenting that this was not needed. Removing that test results in: [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out and the kernel hanging, seemingly in an IRQs-off region.
On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > > That is mostly due to the check in hdlcd_crtc_disable() which I should > > remove, I've added it because I was getting a ->disable() hook call > > before any ->enable() was called at startup time. I need to revisit > > this as I remember Daniel was commenting that this was not needed. > > Removing that test results in: > > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out > > and the kernel hanging, seemingly in an IRQs-off region. Right, I need to sort this one out. Are you doing these tests out of some tagged branch that I can get in sync with? Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, Nov 21, 2016 at 01:50:31PM +0000, Liviu Dudau wrote: > On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote: > > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > > > That is mostly due to the check in hdlcd_crtc_disable() which I should > > > remove, I've added it because I was getting a ->disable() hook call > > > before any ->enable() was called at startup time. I need to revisit > > > this as I remember Daniel was commenting that this was not needed. > > > > Removing that test results in: > > > > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out > > > > and the kernel hanging, seemingly in an IRQs-off region. > > Right, I need to sort this one out. Are you doing these tests out of > some tagged branch that I can get in sync with? No, not yet, and some of the changes I have are rather hacky. I do always build my full tree of patches (which is currently running at around 320 patches at the moment) but I never share that entire patch set. However, none of those touch i2c (apart from the ones I've recently posted) and the only patches touching hdlcd are those I've posted so far. Most of the problems I'm finding are through trying basic stuff - I'm not doing anything special or unusual to find them, at the moment quite literally just starting Xorg up and shutting it down. For example, the above was caused by logging in on serial, running: Xorg -terminate -verbose and then hitting ^C. (I have lxdm disabled so systemd boots to VT login prompts on both the "framebuffer" and serial - I don't want Xorg coming up when the machine is booting for its nightly KVM boot tests.) I'm afraid that when I try someone elses code, I have a tendency to find loads of seemingly trivial bugs when I try putting it through some basic tests.
On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > > That is mostly due to the check in hdlcd_crtc_disable() which I should > > remove, I've added it because I was getting a ->disable() hook call > > before any ->enable() was called at startup time. I need to revisit > > this as I remember Daniel was commenting that this was not needed. > > Removing that test results in: > > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out > > and the kernel hanging, seemingly in an IRQs-off region. Annoyingly, enabling DRM debug prevents the kernel hanging...
On Mon, Nov 21, 2016 at 02:30:53PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote: > > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > > > That is mostly due to the check in hdlcd_crtc_disable() which I should > > > remove, I've added it because I was getting a ->disable() hook call > > > before any ->enable() was called at startup time. I need to revisit > > > this as I remember Daniel was commenting that this was not needed. > > > > Removing that test results in: > > > > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out > > > > and the kernel hanging, seemingly in an IRQs-off region. > > Annoyingly, enabling DRM debug prevents the kernel hanging... I've been trying to trace through what's happening with this flip_done stuff, but I'm finding it _extremely_ difficult to follow the atomic code. (Sorry, I'm going to go over my usual 72 column limit for this due to the damn long DRM function names.) I can see that drm_atomic_helper_commit() calls drm_atomic_helper_setup_commit() which sets up commit->flip_done for each CRTC, and sets up an event for each. drm_atomic_helper_commit() continues on to eventually call drm_atomic_helper_swap_state() which then swaps the state for the CRTCs, but then ends up dropping the event reference: state->crtcs[i].commit->event = NULL; What I can't see is why this isn't a leaked pointer - I don't see anything inbetween taking charge of that structure. The _commit_ hasn't been swapped from what I can see, it's just state->crtcs[i].state that have been swapped. So I can't see who's responsible for generating this event, or how the backend DRM drivers get to know about this event, and that they should complete the flip. What I also don't get is why DRM is wanting to wait for a flip event when we're disabling the CRTC. None of this makes sense to me, like much of the atomic modeset code... (I'm probably never going to convert Armada DRM to atomic modeset, I just don't seem to be capable of understanding atomic modeset. Maybe I'm too old?) [drm:drm_ioctl] pid=2178, dev=0xe200, auth=1, DRM_IOCTL_MODE_RMFB [drm:drm_mode_object_unreference] OBJ ID: 38 (4) [drm:drm_atomic_state_init] Allocated atomic state ffffffc974c7c300 [drm:drm_mode_object_reference] OBJ ID: 43 (1) [drm:drm_atomic_get_crtc_state] Added [CRTC:24:crtc-0] ffffffc975e59400 state to ffffffc974c7c300 [drm:drm_mode_object_reference] OBJ ID: 38 (3) [drm:drm_atomic_get_plane_state] Added [PLANE:23:plane-0] ffffffc974c7c100 state to ffffffc974c7c300 [drm:drm_mode_object_unreference] OBJ ID: 43 (2) [drm:drm_atomic_set_mode_for_crtc] Set [NOMODE] for CRTC state ffffffc975e59400 [drm:drm_atomic_set_crtc_for_plane] Link plane state ffffffc974c7c100 to [NOCRTC] [drm:drm_mode_object_unreference] OBJ ID: 38 (4) [drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane state ffffffc974c7c100 [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:24:crtc-0] to ffffffc974c7c300 [drm:drm_mode_object_reference] OBJ ID: 26 (4) [drm:drm_mode_object_reference] OBJ ID: 26 (5) [drm:drm_atomic_get_connector_state] Added [CONNECTOR:26] ffffffc974c7c000 state to ffffffc974c7c300 [drm:drm_mode_object_unreference] OBJ ID: 26 (6) [drm:drm_atomic_set_crtc_for_connector] Link connector state ffffffc974c7c000 to [NOCRTC] [drm:drm_atomic_check_only] checking ffffffc974c7c300 [drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] mode changed [drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] enable changed [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:26:HDMI-A-1] [drm:drm_atomic_helper_check_modeset] Disabling [CONNECTOR:26:HDMI-A-1] [drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] active changed [drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] needs all connectors, enable: n, active: n [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:24:crtc-0] to ffffffc974c7c300 [drm:drm_atomic_commit] commiting ffffffc974c7c300 [drm:drm_atomic_helper_commit_modeset_disables] disabling [ENCODER:25:TMDS-25] [drm:drm_atomic_helper_commit_modeset_disables] disabling [CRTC:24:crtc-0] hdlcd_crtc_disable: active 0 [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out [drm:drm_atomic_state_default_clear] Clearing atomic state ffffffc974c7c300 [drm:drm_mode_object_unreference] OBJ ID: 26 (5) [drm:drm_mode_object_unreference] OBJ ID: 26 (4) [drm:drm_mode_object_unreference] OBJ ID: 43 (1) [drm:drm_mode_object_unreference] OBJ ID: 38 (3) [drm:drm_atomic_state_free] Freeing atomic state ffffffc974c7c300 [drm:drm_mode_object_unreference] OBJ ID: 38 (2) [drm:drm_mode_object_unreference] OBJ ID: 38 (1)
On Mon, Nov 21, 2016 at 02:55:28PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 21, 2016 at 02:30:53PM +0000, Russell King - ARM Linux wrote: > > On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote: > > > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote: > > > > That is mostly due to the check in hdlcd_crtc_disable() which I should > > > > remove, I've added it because I was getting a ->disable() hook call > > > > before any ->enable() was called at startup time. I need to revisit > > > > this as I remember Daniel was commenting that this was not needed. > > > > > > Removing that test results in: > > > > > > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out > > > > > > and the kernel hanging, seemingly in an IRQs-off region. > > > > Annoyingly, enabling DRM debug prevents the kernel hanging... > > I've been trying to trace through what's happening with this flip_done > stuff, but I'm finding it _extremely_ difficult to follow the atomic > code. > > (Sorry, I'm going to go over my usual 72 column limit for this due to > the damn long DRM function names.) > > I can see that drm_atomic_helper_commit() calls drm_atomic_helper_setup_commit() > which sets up commit->flip_done for each CRTC, and sets up an event for > each. > > drm_atomic_helper_commit() continues on to eventually call drm_atomic_helper_swap_state() > which then swaps the state for the CRTCs, but then ends up dropping > the event reference: > > state->crtcs[i].commit->event = NULL; > > What I can't see is why this isn't a leaked pointer - I don't see > anything inbetween taking charge of that structure. The _commit_ > hasn't been swapped from what I can see, it's just state->crtcs[i].state > that have been swapped. The event is also stored in crtc_state->event, which after swap_states land in drm_crtc->state->event, which is the place drivers are supposed to pick it up from for delivery. > So I can't see who's responsible for generating this event, or how the > backend DRM drivers get to know about this event, and that they should > complete the flip. > > What I also don't get is why DRM is wanting to wait for a flip event > when we're disabling the CRTC. None of this makes sense to me, like > much of the atomic modeset code... The DRM event has two uses: - high-precision timestamp for when the new frame starts displaying. - confirmation that the old buffers are no longer being used by the hw. This is used on Android's drm_hwcomposer in the new hwc2 mode. Note that the crtc_state->event has 3 uses in total, all hidden behind the abstraction: - flip_done, for the atomic helpers - drm event, for current userspace (also needed to emulate legacy flips) - and out-fences, needed by android. The trouble with ->event delivery was that many drivers didn't bother to implement this at all, since driver submitters never even tested pageflippping. And for those maintainers that did test pageflipping, they only ever tested the legacy page_flip paths, which e.g. doesn't ever ask for an event when disabling the CRTC (since you can't do that). But atomic allows all this, and review wasn't enough to fight the influx of bad drivers. Hence I opted to make the nonblocking support in the atomic helpers enforce this part of the abi contract, even for blocking modesets. If you're stuck on a flip_done, then your driver doesn't send out events when disabling the CRTC. All the waits have a 10s timeout, and none of them are in atomic contexts, so no idea why this takes down your box. I suspect it's something unrelated. -Daniel
On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote: > Something I also noticed is this: > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > plane->state->crtc_y * plane->state->fb->pitches[0] + > plane->state->crtc_x * bpp / 8; > > Surely this should be using src_[xy] (which are the position in the > source - iow, memory, and not crtc_[xy] which is the position on the > CRTC displayed window. To put it another way, the src_* define the > region of the source material that is mapped onto a rectangular area > on the display defined by crtc_*. This problem still exists...
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c index 48019ae22ddb..3e97acf6e2a7 100644 --- a/drivers/gpu/drm/arm/hdlcd_crtc.c +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc) clk_prepare_enable(hdlcd->clk); hdlcd_crtc_mode_set_nofb(crtc); hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); } static void hdlcd_crtc_disable(struct drm_crtc *crtc)
Hi, While testing HDMI with Xorg on the Juno board, I find that when Xorg starts up or shuts down, the display is shifted significantly to the right and wrapped in the active region. (No sync bars are visible.) The timings are correct, it behaves as if the start address has been shifted many pixels _into_ the framebuffer. This occurs whenever the display mode size is changed - using xrandr in Xorg shows that changing the resolution triggers the problem almost every time, but changing the refresh rate does not. Using devmem2 to disable and re-enable the HDLCD resolves the issue, and repeated disable/enable cycles do not make the issue re-appear. So, I patched the HDLCD to do this, and testing it with Xorg after several repetitions seems to work. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- What I think is going on is that the FIFO or address generator for reading data from the AXI bus is not properly reset when changing the resolution, and the enable-disable-enable cycle causes the HDLCD hardware to sort itself out. It's (eg) significantly out - for example, to properly align the display, I have to program an address of 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels before the real start of the frame buffer. With this patch, a patch to TDA998x to avoid the i2c-designware issue, and xf86-video-armada, I have LXDE running on the Juno. Something I also noticed is this: scanout_start = gem->paddr + plane->state->fb->offsets[0] + plane->state->crtc_y * plane->state->fb->pitches[0] + plane->state->crtc_x * bpp / 8; Surely this should be using src_[xy] (which are the position in the source - iow, memory, and not crtc_[xy] which is the position on the CRTC displayed window. To put it another way, the src_* define the region of the source material that is mapped onto a rectangular area on the display defined by crtc_*. Another note is that since the CRTC can't place the plane in arbitary positions and sizes within the active area, should the atomic_check ensure that crtc_x = crtc_y = 0, and the crtc width/height are the size of the active area? drivers/gpu/drm/arm/hdlcd_crtc.c | 2 ++ 1 file changed, 2 insertions(+)