[BUG] hdlcd gets confused about base address
Liviu Dudau
liviu.dudau at arm.com
Mon Nov 21 04:56:53 PST 2016
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.
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the linux-arm-kernel
mailing list