[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Russell King - ARM Linux]

On Tue, Nov 22, 2016 at 08:51:11AM +0900, Gustavo Padovan wrote:
> 2016-11-21 Daniel Vetter :
> >  /**
> >   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> > - * @state: new modeset state to be committed
> > + * @old_state: atomic state object with old state structures
> >   *
> >   * This is the default implemenation for the ->atomic_commit_tail() hook 
> > of the
> >   * _mode_config_helper_funcs vtable.
> > @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
> >   *
> >   * For drivers supporting runtime PM the recommended sequence is instead ::
> >   *
> > - * drm_atomic_helper_commit_modeset_disables(dev, state);
> > + * drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >   *
> > - * drm_atomic_helper_commit_modeset_enables(dev, state);
> > + * drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >   *
> > - * drm_atomic_helper_commit_planes(dev, state,
> > + * drm_atomic_helper_commit_planes(dev, old_state,
> >   * DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >   *
> >   * for committing the atomic update to hardware.  See the kerneldoc 
> > entries for
> >   * these three functions for more details.
> >   */
> > -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> > +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> 
> I thought we would commit the new state. Why the rename to old_state?

Because the argument is the _old_ state, and the new state can be
found in the various plane, crtc, etc structures.

-- 
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.

[Bug 98761] [regression][radeonsi][polaris]"radeonsi: set IF_THRESHOLD to 3" breaks Witcher2's ground

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98761

--- Comment #11 from Marek Olšák  ---
First bad commit:

commit 4404d0d6e354e80dd7f8f0a0e12d8ad809cf007e
Author: Matt Arsenault 
Date:   Sun Nov 13 18:20:54 2016 +

AMDGPU: Implement SGPR spilling with scalar stores

nThis avoids the nasty problems caused by using
memory instructions that read the exec mask while
spilling / restoring registers used for control flow
masking, but only for VI when these were added.

This always uses the scalar stores when enabled currently,
but it may be better to still try to spill to a VGPR
and use this on the fallback memory path.

The cache also needs to be flushed before wave termination
if a scalar store is used.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 286766
91177308-0d34-0410-b5e6-96231b3b80d8

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/e7d526d6/attachment-0001.html>

[Bug 188321] New: i915: black screen after disconnecting HDMI monitor

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188321

Bug ID: 188321
   Summary: i915: black screen after disconnecting HDMI monitor
   Product: Drivers
   Version: 2.5
Kernel Version: 4.x
  Hardware: x86-64
OS: Linux
  Tree: Mainline
Status: NEW
  Severity: normal
  Priority: P1
 Component: Video(DRI - non Intel)
  Assignee: drivers_video-dri at kernel-bugs.osdl.org
  Reporter: kernel at stefaus.de
Regression: No

Created attachment 245481
  --> https://bugzilla.kernel.org/attachment.cgi?id=245481=edit
dmesg with drm.debug=14

Since new install of my Linux System and using a 4.4+ Kernel (before 3.19) I'm
getting kernel exceptions every time I disconnect my via HDMI connected
Monitor.

Steps:
- booting to desktop without problems
- disconnect HDMI
- eDP is showing desktop for ~2 seconds
- eDP turns black, no keyboard inputs, the rest keeps running

No difference if HDMI connected while/after boot, I tried different 4.4+ Kernel
Versions from Ubuntu to Mainline, every time nearly the same output.

[   90.275535] [drm:ironlake_crtc_enable [i915]] *ERROR* mode set failed: pipe
A stuck
[   90.329696] [ cut here ]
[   90.329789] WARNING: CPU: 3 PID: 1834 at
/home/kernel/COD/linux/drivers/gpu/drm/i915/intel_display.c:14221
intel_atomic_commit_tail+0xf93/0xfc0 [i915]

see attached dmesg for the full exception trace.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 98005] VCE dual instance encoding inconsistent since st/va: enable dual instances encode by sync surface

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98005

--- Comment #20 from Boyuan Zhang  ---
(In reply to Boyuan Zhang from comment #19)
> (In reply to Boyuan Zhang from comment #18)
> > (In reply to Andy Furniss from comment #14)
> > > but I am afraid the patches regress cbr so the out of order frames near I 
> > > frames issue is back.
> > 
> > The "out of order" issue was caused by sending last p and next i frame
> > together for dual instance encoding, which has already been solved and
> > shouldn't affect by this patch. I suspect the i-frame has less QP (quality)
> > than p-frame in cbr case (to meet constant bit-rate) where bit-rate is not
> > high enough, as a result you might see low picture quality for each i-frame.
> > Please confirm with high bitrate or using analyser to check the order.
> > 
> > If you still see the out-of-order issue, please share the clip and command.
> > 
> > Thanks
> 
> Nevermind, the cbr issue was reproduced and fixed. Please try the latest
> patch I just send.

Links can be found below:
https://lists.freedesktop.org/archives/mesa-dev/2016-November/136065.html
https://lists.freedesktop.org/archives/mesa-dev/2016-November/136066.html

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/0e5f97bd/attachment.html>

[Bug 188091] Resume with two monitors, second monitor is not resumed until VT switch

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188091

--- Comment #4 from Greg White  ---
Note that those two files were grabbed after I switched to a VT (since the UI
was in a pretty whacked state.)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188091] Resume with two monitors, second monitor is not resumed until VT switch

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188091

--- Comment #3 from Greg White  ---
Created attachment 245471
  --> https://bugzilla.kernel.org/attachment.cgi?id=245471=edit
dmesg

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188091] Resume with two monitors, second monitor is not resumed until VT switch

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188091

--- Comment #2 from Greg White  ---
Created attachment 245461
  --> https://bugzilla.kernel.org/attachment.cgi?id=245461=edit
Xorg log

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[PATCH v9 02/10] drm/mediatek: add *driver_data for different hardware settings

[YT Shen]

Hi Daniel,

On Fri, 2016-11-18 at 12:56 +0800, Daniel Kurtz wrote:
> Hi YT,
> 
> I don't see a reason to handle device_data in such a generic way at
> the generic mtk_ddp_comp layer.
> The device data is very component specific, so just define different
> structs for different comp types, ie:
> 
> struct mtk_disp_ovl_driver_data {
> unsigned int reg_ovl_addr;
> unsigned int fmt_rgb565;
> unsigned int fmt_rgb888;
> };
> 
> struct mtk_disp_rdma_driver_data {
> unsigned int fifo_pseudo_size;
> };
> 
> struct mtk_disp_color_driver_data {
> unsigned int color_offset;
> };
> 
> Then add typed pointers to the local structs that use them, for example:
> 
> struct mtk_disp_ovl {
> struct mtk_ddp_comp ddp_comp;
> struct drm_crtc *crtc;
> const struct mtk_disp_ovl_driver_data *data;
> };
> 
> And fetch the device specific driver data directly in .probe, as you
> are already doing:
> 
> static int mtk_disp_ovl_probe(struct platform_device *pdev) {
>   ...
>   priv->data = of_device_get_match_data(dev);
>   ...
> }
These suggestions make code more readable.  We will change ovl and rdma
part, and keep mtk_disp_color_driver_data in its original place.
Because ovl and rdma have its files, other modules share
mtk_drm_ddp_comp.c.

> 
> More comments in-line...
> 
> On Fri, Nov 11, 2016 at 7:55 PM, YT Shen  wrote:
> > There are some hardware settings changed, between MT8173 & MT2701:
> > DISP_OVL address offset changed, color format definition changed.
> > DISP_RDMA fifo size changed.
> > DISP_COLOR offset changed.
> > MIPI_TX pll setting changed.
> > And add prefix for mtk_ddp_main & mtk_ddp_ext & mutex_mod.
> 
> Nit: I think it would make sense to combine this patch with
> drm/mediatek: rename macros, add chip prefix
Will do.

> 
> >
> > Signed-off-by: YT Shen 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 27 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 11 +--
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp.c  | 11 +++
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 
> > +--
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 25 ++---
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.h  |  8 
> >  drivers/gpu/drm/mediatek/mtk_mipi_tx.c  | 24 +++-
> >  8 files changed, 115 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c 
> > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index 019b7ca..1139834 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -35,13 +35,10 @@
> >  #define DISP_REG_OVL_PITCH(n)  (0x0044 + 0x20 * (n))
> >  #define DISP_REG_OVL_RDMA_CTRL(n)  (0x00c0 + 0x20 * (n))
> >  #define DISP_REG_OVL_RDMA_GMC(n)   (0x00c8 + 0x20 * (n))
> > -#define DISP_REG_OVL_ADDR(n)   (0x0f40 + 0x20 * (n))
> 
> Also, I would still use the "#define macros", for example
> "DISP_REG_OVL_ADDR offsets, and use the named constant in the
> driver_data:
> 
> #define DISP_REG_OVL_ADDR_MT8173  0x0f40
> 
> (and in a later patch:
> #define DISP_REG_OVL_ADDR_MT2701  0x0040
> )
> 
> Also, I would still use the macro rather than open coding the "0x20 *
> (n)", and just pass 'ovl' to the overlay macros that depend on
> hardware type.
> Something like the following:
> 
> #define DISP_REG_OVL_ADDR(ovl, n)  ((ovl)->data->ovl_addr + 0x20 * (n))
Will use the "#define macros" here.

> 
> >
> >  #defineOVL_RDMA_MEM_GMC0x40402020
> >
> >  #define OVL_CON_BYTE_SWAP  BIT(24)
> > -#define OVL_CON_CLRFMT_RGB565  (0 << 12)
> > -#define OVL_CON_CLRFMT_RGB888  (1 << 12)
> 
> This seems like a really random and unnecessary hardware change.
> Why chip designers, why!!?!?
There are many reasons for software bugs.  Unnecessary hardware change
should be one of them...

> 
> For this one, it seems the polarity is either one way or the other, so
> we can just use a bool to distinguish:
> 
>   bool fmt_rgb565_is_0;
> 
> > +static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = {
> > +   .ovl = { DISP_REG_OVL_ADDR_MT8173, .fmt_rgb565_is_0 = true }
> > +};
> 
> For use at runtime, the defines could become:
> 
> #define OVL_CON_CLRFMT_RGB565(ovl)  ((ovl)->data->fmt_rgb565_is_0 ? 0
> : OVL_CON_CLRFMT_RGB888)
> #define OVL_CON_CLRFMT_RGB888(ovl)  ((ovl)->data->fmt_rgb565_is_0 ?
> OVL_CON_CLRFMT_RGB888 : 0)
OK, will do.

> 
> >  #define OVL_CON_CLRFMT_RGBA(2 << 12)
> >  #define OVL_CON_CLRFMT_ARGB(3 << 12)
> >  #defineOVL_CON_AEN BIT(8)
> > @@ -137,18 +134,18 @@ static void mtk_ovl_layer_off(struct mtk_ddp_comp 
> > *comp, unsigned int idx)
> > writel(0x0, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
> >  }
> >
> > -static unsigned int 

[Bug 188091] Resume with two monitors, second monitor is not resumed until VT switch

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188091

Alex Deucher  changed:

   What|Removed |Added

 CC||alexdeucher at gmail.com

--- Comment #1 from Alex Deucher  ---
Please attach your xorg log and dmesg output.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> +{
> + const struct da8xx_ddrctl_config_knob *knob;
> + const struct da8xx_ddrctl_setting *setting;
> + struct device_node *node;
> + struct resource *res;
> + void __iomem *ddrctl;
> + struct device *dev;
> + u32 reg;
> +
> + dev = >dev;
> + node = dev->of_node;
> +
> + setting = da8xx_ddrctl_get_board_settings();
> + if (!setting) {
> + dev_err(dev, "no settings for board '%s'\n",
> + of_flat_dt_get_machine_name());
> + return -EINVAL;
> + }

This causes a section mismatch because of_flat_dt_get_machine_name() 
has an __init annotation. I did not notice that before, sorry.

It can be fixed with a patch like below:

---8<---
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
index a20e7bbbcbe0..9ca5aab3ac54 100644
--- a/drivers/memory/da8xx-ddrctl.c
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
*da8xx_ddrctl_get_board_settings(void)
return NULL;
 }

+static const char* da8xx_ddrctl_get_machine_name(void)
+{
+   const char *str;
+   int ret;
+
+   ret = of_property_read_string(of_root, "model", );
+   if (ret)
+   ret = of_property_read_string(of_root, "compatible", );
+
+   return str;
+}
+
 static int da8xx_ddrctl_probe(struct platform_device *pdev)
 {
const struct da8xx_ddrctl_config_knob *knob;
@@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
setting = da8xx_ddrctl_get_board_settings();
if (!setting) {
dev_err(dev, "no settings for board '%s'\n",
-   of_flat_dt_get_machine_name());
+   da8xx_ddrctl_get_machine_name());
return -EINVAL;
}
---8<--- 

A similar fix is required for the other driver in this series (patch 
2/5). I need some advise on whether I should introduce a common 
function to get the machine name post kernel boot-up (I cannot see an 
existing one). If yes, any advise on which file it should go into?

Thanks,
Sekhar

[Bug 98005] VCE dual instance encoding inconsistent since st/va: enable dual instances encode by sync surface

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98005

--- Comment #19 from Boyuan Zhang  ---
(In reply to Boyuan Zhang from comment #18)
> (In reply to Andy Furniss from comment #14)
> > but I am afraid the patches regress cbr so the out of order frames near I 
> > frames issue is back.
> 
> The "out of order" issue was caused by sending last p and next i frame
> together for dual instance encoding, which has already been solved and
> shouldn't affect by this patch. I suspect the i-frame has less QP (quality)
> than p-frame in cbr case (to meet constant bit-rate) where bit-rate is not
> high enough, as a result you might see low picture quality for each i-frame.
> Please confirm with high bitrate or using analyser to check the order.
> 
> If you still see the out-of-order issue, please share the clip and command.
> 
> Thanks

Nevermind, the cbr issue was reproduced and fixed. Please try the latest patch
I just send.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/20a63e85/attachment.html>

[PATCH v9 09/10] drm/mediatek: update DSI sub driver flow for sending commands to panel

[YT Shen]

Hi Daniel,

Thanks for the review.

On Fri, 2016-11-18 at 11:21 +0800, Daniel Kurtz wrote:
> Hi YT,
> 
> Sorry for the very late review.
> 
> My biggest problem with this patch is it describes itself as adding
> support for a new use case "DSI -> panel", but makes many changes to
> the existing working flow "DSI -> bridge -> panel".
> If these changes are really needed, or improve the existing flow, I'd
> expect to see those changes added first in a preparatory patch,
> followed by a second smaller, simpler
> patch that adds any additional functionality required to enable the new flow.
We will split this patch into several smaller preparatory patches
necessary in the next version.

> 
> See detailed comments inline.
> 
> 
> On Fri, Nov 11, 2016 at 7:55 PM, YT Shen  wrote:
> >
> > This patch update enable/disable flow of DSI module and MIPI TX module.
> > Original flow works on there is a bridge chip: DSI -> bridge -> panel.
> > In this case: DSI -> panel, the DSI sub driver flow should be updated.
> > We need to initialize DSI first so that we can send commands to panel.
> >
> > Signed-off-by: shaoming chen 
> > Signed-off-by: YT Shen 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 110 
> > ++---
> >  drivers/gpu/drm/mediatek/mtk_mipi_tx.c |  32 +-
> >  2 files changed, 103 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c 
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 860b84f..12a1206 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -94,6 +94,8 @@
> >  #define DSI_RACK   0x84
> >  #define RACK   BIT(0)
> >
> > +#define DSI_MEM_CONTI  0x90
> > +
> >  #define DSI_PHY_LCCON  0x104
> >  #define LC_HS_TX_ENBIT(0)
> >  #define LC_ULPM_EN BIT(1)
> > @@ -126,6 +128,10 @@
> >  #define CLK_HS_POST(0xff << 8)
> >  #define CLK_HS_EXIT(0xff << 16)
> >
> > +#define DSI_VM_CMD_CON 0x130
> > +#define VM_CMD_EN  BIT(0)
> > +#define TS_VFP_EN  BIT(5)
> > +
> >  #define DSI_CMDQ0  0x180
> >  #define CONFIG (0xff << 0)
> >  #define SHORT_PACKET   0
> > @@ -219,12 +225,12 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
> > writel(timcon3, dsi->regs + DSI_PHY_TIMECON3);
> >  }
> >
> > -static void mtk_dsi_enable(struct mtk_dsi *dsi)
> > +static void mtk_dsi_engine_enable(struct mtk_dsi *dsi)
> 
> I don't think we need to change these names.
OK.

> 
> >  {
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, DSI_EN);
> >  }
> >
> > -static void mtk_dsi_disable(struct mtk_dsi *dsi)
> > +static void mtk_dsi_engine_disable(struct mtk_dsi *dsi)
> >  {
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, 0);
> >  }
> > @@ -249,7 +255,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> >  * mipi_ratio is mipi clk coefficient for balance the pixel clk in 
> > mipi.
> >  * we set mipi_ratio is 1.05.
> >  */
> > -   dsi->data_rate = dsi->vm.pixelclock * 3 * 21 / (1 * 1000 * 10);
> > +   dsi->data_rate = dsi->vm.pixelclock * 12 * 21;
> > +   dsi->data_rate /= (dsi->lanes * 1000 * 10);
> > +   dev_info(dev, "set mipitx's data rate: %dMHz\n", dsi->data_rate);
> 
> I don't think we want to spam the log like this.  Use dev_dbg or
> use the DRM_() messaging like elsewhere in this driver?
OK, we will remove logs like this in the patch series.

> 
> >
> > ret = clk_set_rate(dsi->hs_clk, dsi->data_rate * 100);
> > if (ret < 0) {
> > @@ -271,7 +279,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > goto err_disable_engine_clk;
> > }
> >
> > -   mtk_dsi_enable(dsi);
> > +   mtk_dsi_engine_enable(dsi);
> > mtk_dsi_reset_engine(dsi);
> > mtk_dsi_phy_timconfig(dsi);
> >
> > @@ -289,7 +297,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> >  static void mtk_dsi_clk_ulp_mode_enter(struct mtk_dsi *dsi)
> >  {
> > mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, 0);
> > -   mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, 0);
> > +   mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, LC_ULPM_EN);
> 
> What does this change do?
> It looks like a pure bug fix (ie, previoulsy we were'nt actually
> enabling ULP MODE before).
> If so, can you please move it to a separate preliminary patch.
OK.

> 
> >  }
> >
> >  static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)
> > @@ -302,7 +310,7 @@ static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi 
> > *dsi)
> >  static void mtk_dsi_lane0_ulp_mode_enter(struct mtk_dsi *dsi)
> >  {
> > mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_HS_TX_EN, 0);
> > -   mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, 0);
> > +   mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, LD0_ULPM_EN);
> 
> Same here.
> 
> >  

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Chris Wilson]

On Mon, Nov 21, 2016 at 11:00:52AM -0800, Manasi Navare wrote:
> On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > > - Another fallout is that legacy clients will no longer see the
> > > > >   link-status property. And they won't be able to set it through the
> > > > >   SETCRTC ioctl, which would kinda defaut the point. I think the best
> > > > >   solution would be to check for link_status == BAD in
> > > > >   drm_atomic_helper_set_config, and reset it to good automatically for
> > > > >   legacy clients.
> > > > 
> > > > Then how do they know that the kernel demands the modeset? Both a legacy
> > > > and atomic property?
> > > 
> > > I guess we could avoid the filtering of the property for legacy clients.
> > > Definitely not 2 properties, that's silly. Or we teach userspace to go
> > > look for atomic properties.
> > 
> > Well, now that I flushed the gunk out of my brain with some work-out it's
> > a lot easier: ATOMIC on properties is only to hide them from legacy
> > userspace, it doesn't control how it's implement. Which means we can
> > implement it as described above, and non-atomic userspace can still read
> > it. Setting would also work, but since we want to do that as part of
> > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a
> > modeset will happen or not, automagic in there seems reasonable.
> 
> Thanks Daniel for providing the solution alternatives here.
> So after we make it atomic, we would solve the problem of updating the 
> connector_changed
> in atomic_helper_check_modeset function. So in this, who resets the property 
> to GOOD?
> Would this happen in drm_atomic_helper_set_config in both atomic and non 
> atomic cases?
> 
> And in case of non atomic userspace, will it still be able to read 
> link-status as BAD in userspace
> to decide whether it needs to call setcrtc?
> 
> Chris, will any implementation in your patch for link _status change if this 
> is made atomic?

So long at the property remains visible via the GETCONNECTOR ioctl, no.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Enabling peer to peer device transactions for PCIe devices

[Deucher, Alexander]

This is certainly not the first time this has been brought up, but I'd like to 
try and get some consensus on the best way to move this forward.  Allowing 
devices to talk directly improves performance and reduces latency by avoiding 
the use of staging buffers in system memory.  Also in cases where both devices 
are behind a switch, it avoids the CPU entirely.  Most current APIs (DirectGMA, 
PeerDirect, CUDA, HSA) that deal with this are pointer based.  Ideally we'd be 
able to take a CPU virtual address and be able to get to a physical address 
taking into account IOMMUs, etc.  Having struct pages for the memory would 
allow it to work more generally and wouldn't require as much explicit support 
in drivers that wanted to use it.

Some use cases:
1. Storage devices streaming directly to GPU device memory
2. GPU device memory to GPU device memory streaming
3. DVB/V4L/SDI devices streaming directly to GPU device memory
4. DVB/V4L/SDI devices streaming directly to storage devices

Here is a relatively simple example of how this could work for testing.  This 
is obviously not a complete solution.
- Device memory will be registered with Linux memory sub-system by created 
corresponding struct page structures for device memory
- get_user_pages_fast() will  return corresponding struct pages when CPU 
address points to the device memory
- put_page() will deal with struct pages for device memory

Previously proposed solutions and related proposals:
1.P2P DMA
DMA-API/PCI map_peer_resource support for peer-to-peer 
(http://www.spinics.net/lists/linux-pci/msg44560.html)
Pros: Low impact, already largely reviewed.
Cons: requires explicit support in all drivers that want to support it, doesn't 
handle S/G in device memory.

2. ZONE_DEVICE IO
Direct I/O and DMA for persistent memory (https://lwn.net/Articles/672457/)
Add support for ZONE_DEVICE IO memory with struct pages. 
(https://patchwork.kernel.org/patch/8583221/)
Pro: Doesn't waste system memory for ZONE metadata
Cons: CPU access to ZONE metadata slow, may be lost, corrupted on device reset.

3. DMA-BUF
RDMA subsystem DMA-BUF support 
(http://www.spinics.net/lists/linux-rdma/msg38748.html)
Pros: uses existing dma-buf interface
Cons: dma-buf is handle based, requires explicit dma-buf support in drivers.

4. iopmem
iopmem : A block device for PCIe memory (https://lwn.net/Articles/703895/)

5. HMM
Heterogeneous Memory Management 
(http://lkml.iu.edu/hypermail/linux/kernel/1611.2/02473.html)

6. Some new mmap-like interface that takes a userptr and a length and returns a 
dma-buf and offset?

Alex

RfC: MAINTAINERS update for qemu drm drivers.

[Gerd Hoffmann]

  Hi,

> Also I think one shared git repo for all of them would be
> good.

Yep, I'm doing that (see today's drm-qemu pull req @ dri-devel).
But, yes, I can place the git repo link in MAINTAINERS too.

cheers,
  Gerd

RfC: MAINTAINERS update for qemu drm drivers.

[Daniel Vetter]

On Mon, Nov 21, 2016 at 07:24:34PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> I'm busy updating the MAINTAINERS file for the linux kernel, making sure
> I'm listed for all qemu drm drivers (cirrus, bochs, qxl, virtio), so
> patches land in my inbox.
> 
> While being at it:  I'm wondering whenever it makes sense to also
> include the qemu-devel list there.  I think it would be useful, so qemu
> developers can see what is going on without being subscribed to
> dri-devel.

Imo makes sense. Also I think one shared git repo for all of them would be
good. Bigger tree means you have more reasons for regular pull requests,
and that means patches land in drm-next faster. Which I think is good. And
I'm a bit on a crusade against boutique trees, for these reasons ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Bug 98005] VCE dual instance encoding inconsistent since st/va: enable dual instances encode by sync surface

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98005

--- Comment #18 from Boyuan Zhang  ---
(In reply to Andy Furniss from comment #14)
> but I am afraid the patches regress cbr so the out of order frames near I 
> frames issue is back.

The "out of order" issue was caused by sending last p and next i frame together
for dual instance encoding, which has already been solved and shouldn't affect
by this patch. I suspect the i-frame has less QP (quality) than p-frame in cbr
case (to meet constant bit-rate) where bit-rate is not high enough, as a result
you might see low picture quality for each i-frame. Please confirm with high
bitrate or using analyser to check the order.

If you still see the out-of-order issue, please share the clip and command.

Thanks

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/3c865668/attachment.html>

[linux-sunxi] [PATCH v6 0/5] drm: sun8i: Add DE2 HDMI video support

[Ondřej Jirman]

Dne 21.11.2016 v 19:14 Jean-Francois Moine napsal(a):
> On Mon, 21 Nov 2016 01:54:53 +0100
> Ondřej Jirman  wrote:
> 
>> Dne 20.11.2016 v 12:32 Jean-Francois Moine napsal(a):
>>> This patchset series adds HDMI video support to the Allwinner
>>> sun8i SoCs which include the display engine 2 (DE2).
>>> The driver contains the code for the A83T and H3, but it could be
>>> used/extended for other SoCs as the A64, H2 and H5.
>>
>> Hi,
>>
>> I'm trying to test your patches on Orange Pi PC, and I've run into a few
>> issues: (I'm using sunxi-ng with the same patches as last time, to make
>> it work with your driver)
>>
>> 1] I just get pink output on the monitor - there's some signal, but it's
>> pink (or more like magenta).
>>
>> dmesg ouput indicates no error:
>>
>> [1.887823] [drm] Initialized
>> [1.888503] sun8i-de2 100.de-controller: bound
>> 1c0c000.lcd-controller (ops 0xc0a63894)
>> [2.057298] sun8i-de2 100.de-controller: bound 1ee.hdmi (ops
>> 0xc0a63b54)
>> [2.057304] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [2.057307] [drm] No driver support for vblank timestamp query.
>> [2.690862] Console: switching to colour frame buffer device 240x67
>> [2.723059] sun8i-de2 100.de-controller: fb0:  frame buffer device
>   [snip]
> 
> My H3 boards work correctly, except the Orange PI 2 when it cannot read
> the EDID (but it is OK after reboot).
> 
> Did you check if the EDID was correctly read?

EDID is correctly read (I verified that it is the same as with the v5
version of the driver), but there's one difference I noted. v5 says dpms
is Off, while v6 says dpms is On.

> Which resolution do you expect?
> 

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/1453ca53/attachment-0001.sig>

[PATCH] drm: tilcdc: reduce max_width for revision 1

[Jyri Sarha]

On 11/21/16 19:16, Bartosz Golaszewski wrote:
> It has been determined that the highest resolution supported correctly
> by LCDC rev1 is 800x600. Reduce the max_width value for rev1 to 800 in
> crtc_max_width().
> 

I don't think this is the right way to limit the supported video modes.
There is technically there is no such limit, is there?

If memory bandwidth is limiting the functionality of higher resolutions,
you should use "max-bandwidth" tilcdc device-tree property [1].

Cheers,
Jyri

[1] In "Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt":
Optional properties:
 - max-bandwidth: The maximum pixels per second that the memory
   interface / lcd controller combination can sustain



> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index dfe3dd0..9081de5 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -683,7 +683,7 @@ int tilcdc_crtc_max_width(struct drm_crtc *crtc)
>   int max_width = 0;
>  
>   if (priv->rev == 1)
> - max_width = 1024;
> + max_width = 800;
>   else if (priv->rev == 2)
>   max_width = 2048;
>  
> 

RfC: MAINTAINERS update for qemu drm drivers.

[Gerd Hoffmann]

  Hi,

I'm busy updating the MAINTAINERS file for the linux kernel, making sure
I'm listed for all qemu drm drivers (cirrus, bochs, qxl, virtio), so
patches land in my inbox.

While being at it:  I'm wondering whenever it makes sense to also
include the qemu-devel list there.  I think it would be useful, so qemu
developers can see what is going on without being subscribed to
dri-devel.

Comments?

thanks,
  Gerd

[linux-sunxi] [PATCH v6 0/5] drm: sun8i: Add DE2 HDMI video support

[Jean-Francois Moine]

On Mon, 21 Nov 2016 01:54:53 +0100
Ondřej Jirman  wrote:

> Dne 20.11.2016 v 12:32 Jean-Francois Moine napsal(a):
> > This patchset series adds HDMI video support to the Allwinner
> > sun8i SoCs which include the display engine 2 (DE2).
> > The driver contains the code for the A83T and H3, but it could be
> > used/extended for other SoCs as the A64, H2 and H5.
> 
> Hi,
> 
> I'm trying to test your patches on Orange Pi PC, and I've run into a few
> issues: (I'm using sunxi-ng with the same patches as last time, to make
> it work with your driver)
> 
> 1] I just get pink output on the monitor - there's some signal, but it's
> pink (or more like magenta).
> 
> dmesg ouput indicates no error:
> 
> [1.887823] [drm] Initialized
> [1.888503] sun8i-de2 100.de-controller: bound
> 1c0c000.lcd-controller (ops 0xc0a63894)
> [2.057298] sun8i-de2 100.de-controller: bound 1ee.hdmi (ops
> 0xc0a63b54)
> [2.057304] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [2.057307] [drm] No driver support for vblank timestamp query.
> [2.690862] Console: switching to colour frame buffer device 240x67
> [2.723059] sun8i-de2 100.de-controller: fb0:  frame buffer device
[snip]

My H3 boards work correctly, except the Orange PI 2 when it cannot read
the EDID (but it is OK after reboot).

Did you check if the EDID was correctly read?
Which resolution do you expect?

-- 
Ken ar c'hentañ| ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

[PULL] drm-qemu for 4.10

[Gerd Hoffmann]

  Hi Dave,

Here are the drm-qemu updates for 4.10.

please pull,
  Gerd

The following changes since commit
a25f0944ba9b1d8a6813fd6f1a86f1bd59ac25a6:

  Linux 4.9-rc5 (2016-11-13 10:32:32 -0800)

are available in the git repository at:

  git://git.kraxel.org/linux tags/drm-qemu-20161121

for you to fetch changes up to 348a4b6dd77d183ef4ea67673ecf30a09ae3f9d7:

  drm/virtio: allocate some extra bufs (2016-11-15 09:46:48 +0100)


drm/virtio: fix busid in a different way, allocate more vbufs.
drm/qxl: various bugfixes and cleanups,


Christophe Fergeau (7):
  qxl: Mark some internal functions as static
  qxl: Remove unused prototype
  qxl: Add missing '\n' to qxl_io_log() call
  qxl: Call qxl_gem_{init, fini}
  qxl: Remove qxl_bo_init() return value
  qxl: Don't notify userspace when monitors config is unchanged
  qxl: Allow resolution which are not multiple of 8

Gerd Hoffmann (4):
  drm: re-export drm_dev_set_unique
  drm/virtio: fix busid regression
  Revert "drm: virtio: reinstate drm_virtio_set_busid()"
  drm/virtio: allocate some extra bufs

 drivers/gpu/drm/drm_drv.c| 38 -
 drivers/gpu/drm/qxl/qxl_cmd.c|  2 +-
 drivers/gpu/drm/qxl/qxl_display.c| 69 +++-
 drivers/gpu/drm/qxl/qxl_drv.h|  8 +--
 drivers/gpu/drm/qxl/qxl_fb.c |  2 +-
 drivers/gpu/drm/qxl/qxl_gem.c|  3 +-
 drivers/gpu/drm/qxl/qxl_kms.c|  3 +-
 drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 23 
 drivers/gpu/drm/virtio/virtgpu_drv.c |  1 -
 drivers/gpu/drm/virtio/virtgpu_drv.h |  1 -
 drivers/gpu/drm/virtio/virtgpu_vq.c  |  2 +-
 include/drm/drmP.h   |  1 +
 12 files changed, 100 insertions(+), 53 deletions(-)

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 06:23:24PM +, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 05:56:02PM +, Russell King - ARM Linux wrote:
> > For me, the image shift was 100% reproducable.  With the above patch
> > and a call to drm_crtc_vblank_on() in the enable path, it seems to
> > behave correctly - I can alternately switch between 1920x1080 and
> > 1280x1024 and it behaves correctly.  Indeed, my debug prints show that
> > the right thing is happening wrt disabling the controller:
> 
> OK, so I'll take it that you did not also use your patch to fix the base
> plane calculations, or was that included as well in your stack?

It was before that patch - so it was using crtc_x and crtc_y.  However,
I can guarantee that those were both zero (as I've previously
described.)

> > That's more of a generic DRM issue - the CRTC layer doesn't get a
> > look-in when a connector parses the modes supplied from the display,
> > so there's no real way for the CRTC layer to apply any kind of
> > limitations to the available modes, except when a mode is attempted
> > to be set.
> > 
> > I don't want to see an "interlace" DT property introduced for the
> > TDA998x, because that's the wrong approach - it would be adding a
> > property for the needs of the implementation, and not a description
> > of the hardware.
> 
> AFAICT the issue is the fact that while HDLCD could scan out the alternate
> lines with a bit of a convoluted hack, there is no way to tell TDA19988
> to generate the interlaced timings. And no, I'm not advocating introducing
> a DT property as this is a runtime mode, depending on the resolution
> selected by userspace.

The TDA998x doesn't "generate" the timings.  They come from the input
to it, the TDA998x merely tracks where it is within the frame, so it
knows where it can place things like the infoframes and other data.

So, the responsibility for generating the interlaced timings is with
the CRTC.

That means the CRTC needs to not only scan out alternate lines (which
is the easy bit - setting the pitch to twice the value) but it also
needs to be able to adjust the timing of the vertical sync by half
a line.  The HDLCD from what I can see does not support that, the
overall system does not support for interlaced modes.

> > Whether that has any bearing on the reproducability of this or not, I've
> > no idea.
> 
> The one factor that could affect it is the capability of the SCP firmware
> to generate the exact pixel clock for your 1080p mode. If it doesn't, then
> restoring the old mode might lead to an incorrect synchronisation with the
> TDA chip. Current (less than 1.5 years old I guess) SCP firmware has that
> sorted via an hdlcd.dat file that pre-calculates a lot of common pixel clock
> frequencies).

The TDA998x takes the sync signals itself to synchronise with the CRTC,
and the pixel clock had better be synchronous with the data being closed
out of the CRTC otherwise its going to be in violation of the RGB data
setup and hold timings - which will cause random colour errors.

That isn't what's going on here - the image is rock stable, it's just
shifted.

I tried inverting the sync signals from the CRTC to the TDA998x, and
that shifts the display (as I expect, because the TDA998x synchronises
on the transition of the sync signals not on their absolute values) and
at that point I get the black sync bars appearing - again as expected.

Same kind of effect if I swap the horizontal front and back porches.

Of course, adjusting such things necessitates the TDA998x to re-lock
to the CRTC each time something like that changes, and the image
shift remains.

As I described originally, the _only_ two things that solved the image
shift was (a) shifting the framebuffer start address earlier than it
should be, or (b) disabling the CRTC and re-enabling the CRTC.  Both
of those were tried using devmem2 in userspace with no patches to the
HDLCD code over v4.9-rc5.

The only patches that would be in effect are my TDA998x patch stack
(which you've already tested), the i2c-designware patches to sort that
crappy thing out, and a dirty patch to the TDA998x code to read the
EDID in 16 byte chunks [*], so that the i2c-designware crappage never
causes a problem.

* - I'm not submitting this patch, because while it may solve the
EDID reading issue on Juno, it's putting intimate knowledge of
i2c-designware into the TDA998x driver - it's a hack around the
problem, it's not a real fix.  It's possible that there are other
i2c-designware crappages out there which have even smaller FIFOs
which would need us to read in even smaller chunks for reliability.

-- 
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.

[PATCH] drm: tilcdc: reduce max_width for revision 1

[Bartosz Golaszewski]

2016-11-21 18:26 GMT+01:00 Jyri Sarha :
> On 11/21/16 19:16, Bartosz Golaszewski wrote:
>> It has been determined that the highest resolution supported correctly
>> by LCDC rev1 is 800x600. Reduce the max_width value for rev1 to 800 in
>> crtc_max_width().
>>
>
> I don't think this is the right way to limit the supported video modes.
> There is technically there is no such limit, is there?
>
> If memory bandwidth is limiting the functionality of higher resolutions,
> you should use "max-bandwidth" tilcdc device-tree property [1].
>
> Cheers,
> Jyri
>

Will do, thanks.

Bartosz Golaszewski

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 06:16:16PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 05:56:02PM +, Russell King - ARM Linux wrote:
> > For me, the image shift was 100% reproducable.  With the above patch
> > and a call to drm_crtc_vblank_on() in the enable path, it seems to
> > behave correctly - I can alternately switch between 1920x1080 and
> > 1280x1024 and it behaves correctly.  Indeed, my debug prints show that
> > the right thing is happening wrt disabling the controller:
> 
> Here's my version of your patch:

Thanks! I'll add it to my tree and see if David Airlie is happy to push it
this late into the release cycle. Otherwise it is going to end up in linux-next
quickly and then in drm-next before v4.10.

> 
> 8<=
> From: Russell King 
> Subject: [PATCH] drm/arm: hdlcd: fix plane base address update
> 
> 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.
> Further debugging shows that we try to update the controller
> configuration while enabled.
> 
> Alwys ensure that the HDLCD is disabled prior to updating the
> controller timings, and use drm_crtc_vblank_off()/drm_crtc_vblank_on()
> so that DRM knows whether it can expect vblank interrupts.
> 
> Signed-off-by: Russell King 

Acked-by: Liviu Dudau 

> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c 
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index c239616f5334..9d683be2e5d3 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -151,15 +151,14 @@ 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);
> + drm_crtc_vblank_on(crtc);
>  }
>  
>  static void hdlcd_crtc_disable(struct drm_crtc *crtc)
>  {
>   struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
>  
> - if (!crtc->state->active)
> - return;
> -
> + drm_crtc_vblank_off(crtc);
>   hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
>   clk_disable_unprepare(hdlcd->clk);
>  }
> -- 
> 2.7.4
> 
> -- 
> 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!  /
  ---
¯\_(ツ)_/¯

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 05:56:02PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 05:32:32PM +, Liviu Dudau wrote:
> > On Mon, Nov 21, 2016 at 02:03:49PM +, Russell King - ARM Linux wrote:
> > > On Mon, Nov 21, 2016 at 01:50:31PM +, Liviu Dudau wrote:
> > > > On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux 
> > > > wrote:
> > > > > On Mon, Nov 21, 2016 at 12:56:53PM +, 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?
> > 
> > Hi Russell,
> > 
> > > 
> > > 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.
> > 
> > I'm not being able to reproduce your bug conditions. I'm running the 
> > following
> > setup when testing:
> > 
> > - mainline v4.9-rc6
> > - edited the juno-base.dtsi file to disable the hdlcd at 7f60 and
> >   hdmi-transmitter at 70 nodes to remove the second HDMI output from the 
> > test.
> > - patched tda998x_drv.c to set interlace_allowed = 0, see below why
> > - modified the hdlcd_crtc.c file with the following patch:
> > 
> > -8<---
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c 
> > b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > index 48019ae..656dc43 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > @@ -156,9 +156,7 @@ static void hdlcd_crtc_disable(struct drm_crtc *crtc)
> >  {
> > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> >  
> > -   if (!crtc->state->active)
> > -   return;
> > -
> > +   drm_crtc_vblank_off(crtc);
> 
> Don't you need a drm_crtc_vblank_on() call in the enable function?

I do, thanks for calling me on that!

> 
> > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
> > clk_disable_unprepare(hdlcd->clk);
> >  }
> > ->8---
> > 
> > That takes care of the pxlclk refcounting issue you were seeing. I've 
> > started
> > Xorg several times (and yes, I do see EDID checksum error every now and 
> > then,
> > specially when running xrandr). When closing down Xorg I get back the 
> > framebuffer
> > console with the login prompt and no image shifting.
> 
> For me, the image shift was 100% reproducable.  With the above patch
> and a call to drm_crtc_vblank_on() in the enable path, it seems to
> behave correctly - I can alternately switch between 1920x1080 and
> 1280x1024 and it behaves correctly.  Indeed, my debug prints show that
> the right thing is happening wrt disabling the controller:

OK, so I'll take it that you did not also use your patch to fix the base plane
calculations, or was that included as well in your stack?

> 
> [   76.869136] hdlcd_crtc_disable: active 0
> [   76.869159] hdlcd_plane_atomic_update: pitch 7680 lines 1080
> [   76.888983] hdlcd_plane_atomic_update: pitch 5120 lines 1024
> [   76.888995] hdlcd_crtc_enable: active 1 cmd 
> [   85.262451] hdlcd_crtc_disable: active 0
> [   85.262474] hdlcd_plane_atomic_update: pitch 5120 lines 1024
> [   85.286667] hdlcd_plane_atomic_update: pitch 7680 lines 1080
> [   85.286679] hdlcd_crtc_enable: active 1 cmd 
> [   92.658038] hdlcd_crtc_disable: active 0
> [   92.658057] 

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

v2:
- Liviu pointed out that wait_for_fences is even more magic. Leave
that as @state, and document @pre_swap better.
- While at it, patch in header for the reference section.
- Fix spelling issues Russell noticed.

v3: Fix up the @pre_swap note (Liviu): Also s/synchronous/blocking/,
since async flip is something else than non-blocking.

Cc: Liviu Dudau 
Reported-by: Russell King - ARM Linux 
Cc: Russell King - ARM Linux 
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan 
Cc: Maarten Lankhorst 
Cc: Tomeu Vizoso 
Cc: Daniel Stone 
Signed-off-by: Daniel Vetter 
---
 Documentation/gpu/drm-kms-helpers.rst|  3 ++
 drivers/gpu/drm/drm_atomic_helper.c  | 78 ++--
 include/drm/drm_modeset_helper_vtables.h | 12 +++--
 3 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst 
b/Documentation/gpu/drm-kms-helpers.rst
index 4ca77f675967..03040aa14fe8 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -63,6 +63,9 @@ Atomic State Reset and Initialization
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
:doc: atomic state reset and initialization

+Helper Functions Reference
+--
+
 .. kernel-doc:: include/drm/drm_atomic_helper.h
:internal:

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..494680c9056e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
  * @state: atomic state object with old state structures
- * @pre_swap: if true, do an interruptible wait
+ * @pre_swap: If true, do an interruptible wait, and @state is the new state.
+ * Otherwise @state is the old state.
  *
  * For implicit sync, driver should fish the exclusive fence out from the
  * incoming fb's and stash it in the drm_plane_state.  This is called after
  * drm_atomic_helper_swap_state() so it uses the current plane state (and
  * just uses the atomic state to find the changed planes)
  *
+ * Note that @pre_swap is needed since the point where we block for fences 
moves
+ * around depending upon whether an atomic commit is blocking or
+ * non-blocking. For async commit all waiting needs to happen after
+ * drm_atomic_helper_swap_state() is called, but for synchronous commits we 
want
+ * to wait **before** we do anything that can't be easily rolled back. That is
+ * before we call drm_atomic_helper_swap_state().
+ *
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
@@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);

 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * _mode_config_helper_funcs vtable.
@@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- * drm_atomic_helper_commit_modeset_disables(dev, state);
+ * drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- * drm_atomic_helper_commit_modeset_enables(dev, state);
+ * drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- * drm_atomic_helper_commit_planes(dev, state,
+ * drm_atomic_helper_commit_planes(dev, old_state,
  * DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-   struct drm_device *dev = state->dev;
+   struct drm_device *dev = old_state->dev;

-   drm_atomic_helper_commit_modeset_disables(dev, state);
+   drm_atomic_helper_commit_modeset_disables(dev, old_state);

-   drm_atomic_helper_commit_planes(dev, state, 0);
+   drm_atomic_helper_commit_planes(dev, old_state, 0);

-   drm_atomic_helper_commit_modeset_enables(dev, state);
+   drm_atomic_helper_commit_modeset_enables(dev, old_state);

-   drm_atomic_helper_commit_hw_done(state);
+   drm_atomic_helper_commit_hw_done(old_state);

-   drm_atomic_helper_wait_for_vblanks(dev, state);
+   

[PATCH] drm: tilcdc: reduce max_width for revision 1

[Bartosz Golaszewski]

It has been determined that the highest resolution supported correctly
by LCDC rev1 is 800x600. Reduce the max_width value for rev1 to 800 in
crtc_max_width().

Signed-off-by: Bartosz Golaszewski 
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index dfe3dd0..9081de5 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -683,7 +683,7 @@ int tilcdc_crtc_max_width(struct drm_crtc *crtc)
int max_width = 0;

if (priv->rev == 1)
-   max_width = 1024;
+   max_width = 800;
else if (priv->rev == 2)
max_width = 2048;

-- 
2.9.3

[PATCH] drm: tilcdc: reduce max_width for revision 1

[Bartosz Golaszewski]

While debugging the drm_bridge support for revision 1 I noticed the
driver was selecting the 1024x768 resolution as default from the set
retrieved from EDID. The following patch reduces the max_width for
rev1 in tilcdc.

Bartosz Golaszewski (1):
  drm: tilcdc: reduce max_width for revision 1

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.9.3

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 05:56:02PM +, Russell King - ARM Linux wrote:
> For me, the image shift was 100% reproducable.  With the above patch
> and a call to drm_crtc_vblank_on() in the enable path, it seems to
> behave correctly - I can alternately switch between 1920x1080 and
> 1280x1024 and it behaves correctly.  Indeed, my debug prints show that
> the right thing is happening wrt disabling the controller:

Here's my version of your patch:

8<=
From: Russell King 
Subject: [PATCH] drm/arm: hdlcd: fix plane base address update

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.
Further debugging shows that we try to update the controller
configuration while enabled.

Alwys ensure that the HDLCD is disabled prior to updating the
controller timings, and use drm_crtc_vblank_off()/drm_crtc_vblank_on()
so that DRM knows whether it can expect vblank interrupts.

Signed-off-by: Russell King 
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index c239616f5334..9d683be2e5d3 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -151,15 +151,14 @@ 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);
+   drm_crtc_vblank_on(crtc);
 }

 static void hdlcd_crtc_disable(struct drm_crtc *crtc)
 {
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);

-   if (!crtc->state->active)
-   return;
-
+   drm_crtc_vblank_off(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
clk_disable_unprepare(hdlcd->clk);
 }
-- 
2.7.4

-- 
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.

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sudeep Holla]

Hi Robin,

On 21/11/16 17:47, Robin Murphy wrote:
> Hi Bartosz, Sekhar,
>
> On 21/11/16 16:48, Bartosz Golaszewski wrote:

[...]

>> Hi Sekhar,
>>
>> thanks for spotting that.
>>
>> I think we should introduce this function right away, rather than
>> having two static functions doing the same thing. If you don't mind,
>> I'll try to find a good spot for it and send a follow-up series fixing
>> the issue.
>
> As it happens, that was already proposed last week, for much the same
> reason:
>
> http://www.mail-archive.com/linuxppc-dev at lists.ozlabs.org/msg111395.html
>

Thanks for pointing this out, yet another platform to move to the new
API after v4.10.

Hi Shekar, Bartosz,

For v4.10, please continue with the open coding as proposed in this
thread in order to avoid cross tree dependencies. I will rework on the
above patch once v4.10 merge window closes to include all these
occurrence and replace them.

-- 
Regards,
Sudeep

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 05:32:32PM +, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 02:03:49PM +, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2016 at 01:50:31PM +, Liviu Dudau wrote:
> > > On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> > > > On Mon, Nov 21, 2016 at 12:56:53PM +, 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?
> 
> Hi Russell,
> 
> > 
> > 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.
> 
> I'm not being able to reproduce your bug conditions. I'm running the following
> setup when testing:
> 
> - mainline v4.9-rc6
> - edited the juno-base.dtsi file to disable the hdlcd at 7f60 and
>   hdmi-transmitter at 70 nodes to remove the second HDMI output from the test.
> - patched tda998x_drv.c to set interlace_allowed = 0, see below why
> - modified the hdlcd_crtc.c file with the following patch:
> 
> -8<---
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c 
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 48019ae..656dc43 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -156,9 +156,7 @@ static void hdlcd_crtc_disable(struct drm_crtc *crtc)
>  {
>   struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
>  
> - if (!crtc->state->active)
> - return;
> -
> + drm_crtc_vblank_off(crtc);

Don't you need a drm_crtc_vblank_on() call in the enable function?

>   hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
>   clk_disable_unprepare(hdlcd->clk);
>  }
> ->8---
> 
> That takes care of the pxlclk refcounting issue you were seeing. I've started
> Xorg several times (and yes, I do see EDID checksum error every now and then,
> specially when running xrandr). When closing down Xorg I get back the 
> framebuffer
> console with the login prompt and no image shifting.

For me, the image shift was 100% reproducable.  With the above patch
and a call to drm_crtc_vblank_on() in the enable path, it seems to
behave correctly - I can alternately switch between 1920x1080 and
1280x1024 and it behaves correctly.  Indeed, my debug prints show that
the right thing is happening wrt disabling the controller:

[   76.869136] hdlcd_crtc_disable: active 0
[   76.869159] hdlcd_plane_atomic_update: pitch 7680 lines 1080
[   76.888983] hdlcd_plane_atomic_update: pitch 5120 lines 1024
[   76.888995] hdlcd_crtc_enable: active 1 cmd 
[   85.262451] hdlcd_crtc_disable: active 0
[   85.262474] hdlcd_plane_atomic_update: pitch 5120 lines 1024
[   85.286667] hdlcd_plane_atomic_update: pitch 7680 lines 1080
[   85.286679] hdlcd_crtc_enable: active 1 cmd 
[   92.658038] hdlcd_crtc_disable: active 0
[   92.658057] hdlcd_plane_atomic_update: pitch 7680 lines 1080
[   92.680659] hdlcd_plane_atomic_update: pitch 5120 lines 1024
[   92.680668] hdlcd_crtc_enable: active 1 cmd 
[   97.805205] hdlcd_crtc_disable: active 0
[   97.805220] hdlcd_plane_atomic_update: pitch 5120 lines 1024
[   97.834415] hdlcd_plane_atomic_update: pitch 7680 lines 1080
[   97.834423] hdlcd_crtc_enable: active 1 cmd 

> My monitor is a TV that
> reports that preferred mode is 1080i, however HDLCD and TDA19988 

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

v2:
- Liviu pointed out that wait_for_fences is even more magic. Leave
that as @state, and document @pre_swap better.
- While at it, patch in header for the reference section.
- Fix spelling issues Russell noticed.

Cc: Liviu Dudau 
Reported-by: Russell King - ARM Linux 
Cc: Russell King - ARM Linux 
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan 
Cc: Maarten Lankhorst 
Cc: Tomeu Vizoso 
Cc: Daniel Stone 
Signed-off-by: Daniel Vetter 
---
 Documentation/gpu/drm-kms-helpers.rst|  3 ++
 drivers/gpu/drm/drm_atomic_helper.c  | 78 ++--
 include/drm/drm_modeset_helper_vtables.h | 12 +++--
 3 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst 
b/Documentation/gpu/drm-kms-helpers.rst
index 4ca77f675967..03040aa14fe8 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -63,6 +63,9 @@ Atomic State Reset and Initialization
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
:doc: atomic state reset and initialization

+Helper Functions Reference
+--
+
 .. kernel-doc:: include/drm/drm_atomic_helper.h
:internal:

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..86459554ef5f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
  * @state: atomic state object with old state structures
- * @pre_swap: if true, do an interruptible wait
+ * @pre_swap: If true, do an interruptible wait, and @state is the new state.
+ * Otherwise @state is the old state.
  *
  * For implicit sync, driver should fish the exclusive fence out from the
  * incoming fb's and stash it in the drm_plane_state.  This is called after
  * drm_atomic_helper_swap_state() so it uses the current plane state (and
  * just uses the atomic state to find the changed planes)
  *
+ * Note that @pre_swap is needed since we the point where we block for fences
+ * moves around depending upon whether an atomic commit is synchronous or
+ * asynchronous. For async commit all waiting needs to happen after
+ * drm_atomic_helper_swap_state() is called, but for synchronous commits we 
want
+ * to wait _before_ we do anything that can't be easily rolled back. And hence
+ * before we call drm_atomic_helper_swap_state().
+ *
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
@@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);

 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * _mode_config_helper_funcs vtable.
@@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- * drm_atomic_helper_commit_modeset_disables(dev, state);
+ * drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- * drm_atomic_helper_commit_modeset_enables(dev, state);
+ * drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- * drm_atomic_helper_commit_planes(dev, state,
+ * drm_atomic_helper_commit_planes(dev, old_state,
  * DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-   struct drm_device *dev = state->dev;
+   struct drm_device *dev = old_state->dev;

-   drm_atomic_helper_commit_modeset_disables(dev, state);
+   drm_atomic_helper_commit_modeset_disables(dev, old_state);

-   drm_atomic_helper_commit_planes(dev, state, 0);
+   drm_atomic_helper_commit_planes(dev, old_state, 0);

-   drm_atomic_helper_commit_modeset_enables(dev, state);
+   drm_atomic_helper_commit_modeset_enables(dev, old_state);

-   drm_atomic_helper_commit_hw_done(state);
+   drm_atomic_helper_commit_hw_done(old_state);

-   drm_atomic_helper_wait_for_vblanks(dev, state);
+   drm_atomic_helper_wait_for_vblanks(dev, old_state);

-   drm_atomic_helper_cleanup_planes(dev, state);
+   

[PATCH 2/2] drm/nouveau: Queue hpd_work on (runtime) resume

[Hans de Goede]

We need to call drm_helper_hpd_irq_event() on resume to properly detect
monitor connection / disconnection on some laptops, use hpd_work for
this to avoid deadlocks.

Signed-off-by: Hans de Goede 
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 3100fd88..b564ab8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -692,7 +692,12 @@ nouveau_pmops_resume(struct device *dev)
return ret;
pci_set_master(pdev);

-   return nouveau_do_resume(drm_dev, false);
+   ret = nouveau_do_resume(drm_dev, false);
+
+   /* Monitors may have been connected / disconnected during suspend */
+   schedule_work(_drm(drm_dev)->hpd_work);
+
+   return ret;
 }

 static int
@@ -766,6 +771,10 @@ nouveau_pmops_runtime_resume(struct device *dev)
nvif_mask(>object, 0x088488, (1 << 25), (1 << 25));
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_ON);
drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
+
+   /* Monitors may have been connected / disconnected during suspend */
+   schedule_work(_drm(drm_dev)->hpd_work);
+
return ret;
 }

-- 
2.9.3

[PATCH 1/2] drm/nouveau: Rename acpi_work to hpd_work

[Hans de Goede]

We need to call drm_helper_hpd_irq_event() on resume to properly detect
monitor connection / disconnection on some laptops. For runtime-resume
(which gets called on resume from normal suspend too) we must call
drm_helper_hpd_irq_event() from a workqueue to avoid a deadlock.

Rename acpi_work to hpd_work, and move it out of the #ifdef CONFIG_ACPI
blocks to make it suitable for generic work.

Signed-off-by: Hans de Goede 
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 32 +++
 drivers/gpu/drm/nouveau/nouveau_drv.h |  2 +-
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index a0be029..3cd2b8a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -359,21 +359,10 @@ static struct nouveau_drm_prop_enum_list dither_depth[] = 
{
}  \
 } while(0)

-#ifdef CONFIG_ACPI
-
-/*
- * Hans de Goede: This define belongs in acpi/video.h, I've submitted a patch
- * to the acpi subsys to move it there from drivers/acpi/acpi_video.c .
- * This should be dropped once that is merged.
- */
-#ifndef ACPI_VIDEO_NOTIFY_PROBE
-#define ACPI_VIDEO_NOTIFY_PROBE0x81
-#endif
-
 static void
-nouveau_display_acpi_work(struct work_struct *work)
+nouveau_display_hpd_work(struct work_struct *work)
 {
-   struct nouveau_drm *drm = container_of(work, typeof(*drm), acpi_work);
+   struct nouveau_drm *drm = container_of(work, typeof(*drm), hpd_work);

pm_runtime_get_sync(drm->dev->dev);

@@ -383,6 +372,17 @@ nouveau_display_acpi_work(struct work_struct *work)
pm_runtime_put_sync(drm->dev->dev);
 }

+#ifdef CONFIG_ACPI
+
+/*
+ * Hans de Goede: This define belongs in acpi/video.h, I've submitted a patch
+ * to the acpi subsys to move it there from drivers/acpi/acpi_video.c .
+ * This should be dropped once that is merged.
+ */
+#ifndef ACPI_VIDEO_NOTIFY_PROBE
+#define ACPI_VIDEO_NOTIFY_PROBE0x81
+#endif
+
 static int
 nouveau_display_acpi_ntfy(struct notifier_block *nb, unsigned long val,
  void *data)
@@ -395,9 +395,9 @@ nouveau_display_acpi_ntfy(struct notifier_block *nb, 
unsigned long val,
/*
 * This may be the only indication we receive of a
 * connector hotplug on a runtime suspended GPU,
-* schedule acpi_work to check.
+* schedule hpd_work to check.
 */
-   schedule_work(>acpi_work);
+   schedule_work(>hpd_work);

/* acpi-video should not generate keypresses for this */
return NOTIFY_BAD;
@@ -587,8 +587,8 @@ nouveau_display_create(struct drm_device *dev)
}

nouveau_backlight_init(dev);
+   INIT_WORK(>hpd_work, nouveau_display_hpd_work);
 #ifdef CONFIG_ACPI
-   INIT_WORK(>acpi_work, nouveau_display_acpi_work);
drm->acpi_nb.notifier_call = nouveau_display_acpi_ntfy;
register_acpi_notifier(>acpi_nb);
 #endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 71d4532..0c17ca1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -163,9 +163,9 @@ struct nouveau_drm {
struct nvbios vbios;
struct nouveau_display *display;
struct backlight_device *backlight;
+   struct work_struct hpd_work;
 #ifdef CONFIG_ACPI
struct notifier_block acpi_nb;
-   struct work_struct acpi_work;
 #endif

/* power management */
-- 
2.9.3

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

2016-11-21 17:33 GMT+01:00 Sekhar Nori :
> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> + const struct da8xx_ddrctl_config_knob *knob;
>> + const struct da8xx_ddrctl_setting *setting;
>> + struct device_node *node;
>> + struct resource *res;
>> + void __iomem *ddrctl;
>> + struct device *dev;
>> + u32 reg;
>> +
>> + dev = >dev;
>> + node = dev->of_node;
>> +
>> + setting = da8xx_ddrctl_get_board_settings();
>> + if (!setting) {
>> + dev_err(dev, "no settings for board '%s'\n",
>> + of_flat_dt_get_machine_name());
>> + return -EINVAL;
>> + }
>
> This causes a section mismatch because of_flat_dt_get_machine_name()
> has an __init annotation. I did not notice that before, sorry.
>
> It can be fixed with a patch like below:
>
> ---8<---
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> index a20e7bbbcbe0..9ca5aab3ac54 100644
> --- a/drivers/memory/da8xx-ddrctl.c
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
> *da8xx_ddrctl_get_board_settings(void)
> return NULL;
>  }
>
> +static const char* da8xx_ddrctl_get_machine_name(void)
> +{
> +   const char *str;
> +   int ret;
> +
> +   ret = of_property_read_string(of_root, "model", );
> +   if (ret)
> +   ret = of_property_read_string(of_root, "compatible", );
> +
> +   return str;
> +}
> +
>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  {
> const struct da8xx_ddrctl_config_knob *knob;
> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
> *pdev)
> setting = da8xx_ddrctl_get_board_settings();
> if (!setting) {
> dev_err(dev, "no settings for board '%s'\n",
> -   of_flat_dt_get_machine_name());
> +   da8xx_ddrctl_get_machine_name());
> return -EINVAL;
> }
> ---8<---
>
> A similar fix is required for the other driver in this series (patch
> 2/5). I need some advise on whether I should introduce a common
> function to get the machine name post kernel boot-up (I cannot see an
> existing one). If yes, any advise on which file it should go into?
>

Hi Sekhar,

thanks for spotting that.

I think we should introduce this function right away, rather than
having two static functions doing the same thing. If you don't mind,
I'll try to find a good spot for it and send a follow-up series fixing
the issue.

Best regards,
Bartosz Golaszewski

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Robin Murphy]

Hi Bartosz, Sekhar,

On 21/11/16 16:48, Bartosz Golaszewski wrote:
> 2016-11-21 17:33 GMT+01:00 Sekhar Nori :
>> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> +{
>>> + const struct da8xx_ddrctl_config_knob *knob;
>>> + const struct da8xx_ddrctl_setting *setting;
>>> + struct device_node *node;
>>> + struct resource *res;
>>> + void __iomem *ddrctl;
>>> + struct device *dev;
>>> + u32 reg;
>>> +
>>> + dev = >dev;
>>> + node = dev->of_node;
>>> +
>>> + setting = da8xx_ddrctl_get_board_settings();
>>> + if (!setting) {
>>> + dev_err(dev, "no settings for board '%s'\n",
>>> + of_flat_dt_get_machine_name());
>>> + return -EINVAL;
>>> + }
>>
>> This causes a section mismatch because of_flat_dt_get_machine_name()
>> has an __init annotation. I did not notice that before, sorry.
>>
>> It can be fixed with a patch like below:
>>
>> ---8<---
>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>> --- a/drivers/memory/da8xx-ddrctl.c
>> +++ b/drivers/memory/da8xx-ddrctl.c
>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
>> *da8xx_ddrctl_get_board_settings(void)
>> return NULL;
>>  }
>>
>> +static const char* da8xx_ddrctl_get_machine_name(void)
>> +{
>> +   const char *str;
>> +   int ret;
>> +
>> +   ret = of_property_read_string(of_root, "model", );
>> +   if (ret)
>> +   ret = of_property_read_string(of_root, "compatible", );
>> +
>> +   return str;
>> +}
>> +
>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  {
>> const struct da8xx_ddrctl_config_knob *knob;
>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
>> *pdev)
>> setting = da8xx_ddrctl_get_board_settings();
>> if (!setting) {
>> dev_err(dev, "no settings for board '%s'\n",
>> -   of_flat_dt_get_machine_name());
>> +   da8xx_ddrctl_get_machine_name());
>> return -EINVAL;
>> }
>> ---8<---
>>
>> A similar fix is required for the other driver in this series (patch
>> 2/5). I need some advise on whether I should introduce a common
>> function to get the machine name post kernel boot-up (I cannot see an
>> existing one). If yes, any advise on which file it should go into?
>>
> 
> Hi Sekhar,
> 
> thanks for spotting that.
> 
> I think we should introduce this function right away, rather than
> having two static functions doing the same thing. If you don't mind,
> I'll try to find a good spot for it and send a follow-up series fixing
> the issue.

As it happens, that was already proposed last week, for much the same
reason:

http://www.mail-archive.com/linuxppc-dev at lists.ozlabs.org/msg111395.html

Robin.

> 
> Best regards,
> Bartosz Golaszewski
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

Hi Sekhar,

(And adding Sudeep since he becomes involved in this further
down thread and at that point says he will re-work this
proposed work around in a manner that is incorrect in a
manner that is similar to this proposed work around.)

On 11/21/16 08:33, Sekhar Nori wrote:
> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +const struct da8xx_ddrctl_config_knob *knob;
>> +const struct da8xx_ddrctl_setting *setting;
>> +struct device_node *node;
>> +struct resource *res;
>> +void __iomem *ddrctl;
>> +struct device *dev;
>> +u32 reg;
>> +
>> +dev = >dev;
>> +node = dev->of_node;
>> +
>> +setting = da8xx_ddrctl_get_board_settings();
>> +if (!setting) {
>> +dev_err(dev, "no settings for board '%s'\n",
>> +of_flat_dt_get_machine_name());
>> +return -EINVAL;
>> +}
> 
> This causes a section mismatch because of_flat_dt_get_machine_name() 
> has an __init annotation. I did not notice that before, sorry.
> 
> It can be fixed with a patch like below:
> 
> ---8<---
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> index a20e7bbbcbe0..9ca5aab3ac54 100644
> --- a/drivers/memory/da8xx-ddrctl.c
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
> *da8xx_ddrctl_get_board_settings(void)
>   return NULL;
>  }
>  
> +static const char* da8xx_ddrctl_get_machine_name(void)
> +{
> + const char *str;
> + int ret;
> +
> + ret = of_property_read_string(of_root, "model", );
> + if (ret)
> + ret = of_property_read_string(of_root, "compatible", );
> +
> + return str;
> +}
> +
>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  {
>   const struct da8xx_ddrctl_config_knob *knob;
> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
> *pdev)
>   setting = da8xx_ddrctl_get_board_settings();
>   if (!setting) {
>   dev_err(dev, "no settings for board '%s'\n",
> - of_flat_dt_get_machine_name());

da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
property in the root node.  The "model" property in the root node has
nothing to do with the failure to match. So creating and then using
da8xx_ddrctl_get_machine_name() to potentially report model is not useful.

It should be sufficient to simply report that no compatible matched.


> + da8xx_ddrctl_get_machine_name());
>   return -EINVAL;
>   }
> ---8<--- 
> 
> A similar fix is required for the other driver in this series (patch 
> 2/5). I need some advise on whether I should introduce a common 
> function to get the machine name post kernel boot-up (I cannot see an 
> existing one). If yes, any advise on which file it should go into?
> 
> Thanks,
> Sekhar
> 
> 

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

Cc: Liviu Dudau 
Reported-by: Russell King - ARM Linux 
Cc: Russell King - ARM Linux 
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan 
Cc: Maarten Lankhorst 
Cc: Tomeu Vizoso 
Cc: Daniel Stone 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_atomic_helper.c  | 66 
 include/drm/drm_modeset_helper_vtables.h | 12 --
 2 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..94cde2d3a2ce 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1005,7 +1005,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 /**
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
- * @state: atomic state object with old state structures
+ * @old_state: atomic state object with old state structures
  * @pre_swap: if true, do an interruptible wait
  *
  * For implicit sync, driver should fish the exclusive fence out from the
@@ -1016,14 +1016,14 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
- struct drm_atomic_state *state,
+ struct drm_atomic_state *old_state,
  bool pre_swap)
 {
struct drm_plane *plane;
struct drm_plane_state *plane_state;
int i, ret;

-   for_each_plane_in_state(state, plane, plane_state, i) {
+   for_each_plane_in_state(old_state, plane, plane_state, i) {
if (!pre_swap)
plane_state = plane->state;

@@ -1147,7 +1147,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);

 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * _mode_config_helper_funcs vtable.
@@ -1158,53 +1158,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- * drm_atomic_helper_commit_modeset_disables(dev, state);
+ * drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- * drm_atomic_helper_commit_modeset_enables(dev, state);
+ * drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- * drm_atomic_helper_commit_planes(dev, state,
+ * drm_atomic_helper_commit_planes(dev, old_state,
  * DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-   struct drm_device *dev = state->dev;
+   struct drm_device *dev = old_state->dev;

-   drm_atomic_helper_commit_modeset_disables(dev, state);
+   drm_atomic_helper_commit_modeset_disables(dev, old_state);

-   drm_atomic_helper_commit_planes(dev, state, 0);
+   drm_atomic_helper_commit_planes(dev, old_state, 0);

-   drm_atomic_helper_commit_modeset_enables(dev, state);
+   drm_atomic_helper_commit_modeset_enables(dev, old_state);

-   drm_atomic_helper_commit_hw_done(state);
+   drm_atomic_helper_commit_hw_done(old_state);

-   drm_atomic_helper_wait_for_vblanks(dev, state);
+   drm_atomic_helper_wait_for_vblanks(dev, old_state);

-   drm_atomic_helper_cleanup_planes(dev, state);
+   drm_atomic_helper_cleanup_planes(dev, old_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);

-static void commit_tail(struct drm_atomic_state *state)
+static void commit_tail(struct drm_atomic_state *old_state)
 {
-   struct drm_device *dev = state->dev;
+   struct drm_device *dev = old_state->dev;
struct drm_mode_config_helper_funcs *funcs;

funcs = dev->mode_config.helper_private;

-   drm_atomic_helper_wait_for_fences(dev, state, false);
+   drm_atomic_helper_wait_for_fences(dev, old_state, false);

-   drm_atomic_helper_wait_for_dependencies(state);
+   drm_atomic_helper_wait_for_dependencies(old_state);

if (funcs && funcs->atomic_commit_tail)
-   funcs->atomic_commit_tail(state);
+   funcs->atomic_commit_tail(old_state);
else
-   drm_atomic_helper_commit_tail(state);
+  

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 02:03:49PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 01:50:31PM +, Liviu Dudau wrote:
> > On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> > > On Mon, Nov 21, 2016 at 12:56:53PM +, 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?

Hi Russell,

> 
> 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.

I'm not being able to reproduce your bug conditions. I'm running the following
setup when testing:

- mainline v4.9-rc6
- edited the juno-base.dtsi file to disable the hdlcd at 7f60 and
  hdmi-transmitter at 70 nodes to remove the second HDMI output from the test.
- patched tda998x_drv.c to set interlace_allowed = 0, see below why
- modified the hdlcd_crtc.c file with the following patch:

-8<---
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 48019ae..656dc43 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -156,9 +156,7 @@ static void hdlcd_crtc_disable(struct drm_crtc *crtc)
 {
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);

-   if (!crtc->state->active)
-   return;
-
+   drm_crtc_vblank_off(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
clk_disable_unprepare(hdlcd->clk);
 }
->8---

That takes care of the pxlclk refcounting issue you were seeing. I've started
Xorg several times (and yes, I do see EDID checksum error every now and then,
specially when running xrandr). When closing down Xorg I get back the 
framebuffer
console with the login prompt and no image shifting. My monitor is a TV that
reports that preferred mode is 1080i, however HDLCD and TDA19988 don't talk
propertly with each other to be able to set the interlaced mode correctly, so
I've had to disable support for interlacing mode in tda998x_drv.c and now the
preferred mode that gets picked up is 1920x1200 at 60Hz.

Please advise on what other steps I can take to try to reproduce this.

P.S: What revision of Juno do you have? Any chance you can capture the start
of the boot process where the firmware component prints the version numbers?

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.

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #6 from Vadim Markovtsev  ---
Created attachment 245411
  --> https://bugzilla.kernel.org/attachment.cgi?id=245411=edit
uname -a

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #5 from Vadim Markovtsev  ---
Created attachment 245401
  --> https://bugzilla.kernel.org/attachment.cgi?id=245401=edit
cat /proc/cmdline

Added intel_iommu=off

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #4 from Vadim Markovtsev  ---
Created attachment 245391
  --> https://bugzilla.kernel.org/attachment.cgi?id=245391=edit
nvidia-smi proto -m

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #3 from Vadim Markovtsev  ---
Created attachment 245381
  --> https://bugzilla.kernel.org/attachment.cgi?id=245381=edit
lspci -knnv

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #2 from Vadim Markovtsev  ---
Created attachment 245371
  --> https://bugzilla.kernel.org/attachment.cgi?id=245371=edit
lscpu

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

--- Comment #1 from Vadim Markovtsev  ---
Created attachment 245361
  --> https://bugzilla.kernel.org/attachment.cgi?id=245361=edit
dmidecode -t 2

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 188271] New: IOMMU DMAR fault with NVIDIA CUDA peer to peer

[bugzilla-dae...@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=188271

Bug ID: 188271
   Summary: IOMMU DMAR fault with NVIDIA CUDA peer to peer
   Product: Drivers
   Version: 2.5
Kernel Version: 4.8.6
  Hardware: x86-64
OS: Linux
  Tree: Mainline
Status: NEW
  Severity: normal
  Priority: P1
 Component: Video(DRI - non Intel)
  Assignee: drivers_video-dri at kernel-bugs.osdl.org
  Reporter: vadim at sourced.tech
Regression: No

My motherboard is Supermicro X10DRG-Q (details in attached output of
dmidecode). It has 2 Xeon E5-2620 v4 (details in attached lscpu output). Two
Titan X 2016 GPUs are inserted into PCIe slots (see nvidia-smi output). After
enabling of the peer to peer access between those two cards, execution of
cudaMemcpyPeer() hangs and dmesg shows:

[16193.612535] DMAR: DRHD: handling fault status reg 602
[16193.617662] DMAR: [DMA Write] Request device [82:00.0] fault addr
387fc000c000 [fault reason 05] PTE Write access is not set
[16193.661857] DMAR: DRHD: handling fault status reg 702
[16193.666976] DMAR: [DMA Write] Request device [82:00.0] fault addr f8139000
[fault reason 05] PTE Write access is not set (edited)

I am using CoreOS, and the whole stuff happens inside a docker container
running with -device /dev/nvidiactl --device /dev/nvidia0 --device /dev/nvidia1
--device /dev/nvidia-uvm --privileged --security-opt seccomp=unconfined

The addition of intel_iommu=igfx_off to kernel command line cures the problem
and peer to peer works perfectly.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Liviu Dudau]

On Mon, Nov 21, 2016 at 05:52:57PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.
> 
> v2:

I feel a v3 coming soon :)

> - Liviu pointed out that wait_for_fences is even more magic. Leave
> that as @state, and document @pre_swap better.
> - While at it, patch in header for the reference section.
> - Fix spelling issues Russell noticed.
> 
> Cc: Liviu Dudau 
> Reported-by: Russell King - ARM Linux 
> Cc: Russell King - ARM Linux 
> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
> Cc: Gustavo Padovan 
> Cc: Maarten Lankhorst 
> Cc: Tomeu Vizoso 
> Cc: Daniel Stone 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/drm-kms-helpers.rst|  3 ++
>  drivers/gpu/drm/drm_atomic_helper.c  | 78 
> ++--
>  include/drm/drm_modeset_helper_vtables.h | 12 +++--
>  3 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst 
> b/Documentation/gpu/drm-kms-helpers.rst
> index 4ca77f675967..03040aa14fe8 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -63,6 +63,9 @@ Atomic State Reset and Initialization
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
> :doc: atomic state reset and initialization
>  
> +Helper Functions Reference
> +--
> +
>  .. kernel-doc:: include/drm/drm_atomic_helper.h
> :internal:
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 0b16587cdc62..86459554ef5f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1006,13 +1006,21 @@ 
> EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
>   * @dev: DRM device
>   * @state: atomic state object with old state structures
> - * @pre_swap: if true, do an interruptible wait
> + * @pre_swap: If true, do an interruptible wait, and @state is the new state.
> + *   Otherwise @state is the old state.
>   *
>   * For implicit sync, driver should fish the exclusive fence out from the
>   * incoming fb's and stash it in the drm_plane_state.  This is called after
>   * drm_atomic_helper_swap_state() so it uses the current plane state (and
>   * just uses the atomic state to find the changed planes)
>   *
> + * Note that @pre_swap is needed since we the point where we block for fences

confused about 'we the point' in there. Feels like you were trying to say 
something else?

> + * moves around depending upon whether an atomic commit is synchronous or
> + * asynchronous. For async commit all waiting needs to happen after
> + * drm_atomic_helper_swap_state() is called, but for synchronous commits we 
> want
> + * to wait _before_ we do anything that can't be easily rolled back. And 
> hence

s/And hence/That is/

Otherwise, it looks good to me.

Best regards,
Liviu

> + * before we call drm_atomic_helper_swap_state().
> + *
>   * Returns zero if success or < 0 if dma_fence_wait() fails.
>   */
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> @@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  
>  /**
>   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This is the default implemenation for the ->atomic_commit_tail() hook of 
> the
>   * _mode_config_helper_funcs vtable.
> @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   *
>   * For drivers supporting runtime PM the recommended sequence is instead ::
>   *
> - * drm_atomic_helper_commit_modeset_disables(dev, state);
> + * drm_atomic_helper_commit_modeset_disables(dev, old_state);
>   *
> - * drm_atomic_helper_commit_modeset_enables(dev, state);
> + * drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   *
> - * drm_atomic_helper_commit_planes(dev, state,
> + * drm_atomic_helper_commit_planes(dev, old_state,
>   * DRM_PLANE_COMMIT_ACTIVE_ONLY);
>   *
>   * for committing the atomic update to hardware.  See the kerneldoc entries 
> for
>   * these three functions for more details.
>   */
> -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
>  {
> - struct drm_device *dev = state->dev;
> + struct drm_device *dev = old_state->dev;
>  
> - drm_atomic_helper_commit_modeset_disables(dev, state);
> + drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
> - drm_atomic_helper_commit_planes(dev, state, 0);
> +   

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Daniel Vetter]

On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst 
> > > > > > > > > wrote:
> > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > >>> if connector link status property has changed. This will 
> > > > > > > > >>> tell the
> > > > > > > > >>> driver to do a complete modeset due to change in connector 
> > > > > > > > >>> property.
> > > > > > > > >>>
> > > > > > > > >>> Acked-by: Harry Wentland 
> > > > > > > > >>> Acked-by: Tony Cheng 
> > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > >>> Cc: Jani Nikula 
> > > > > > > > >>> Cc: Daniel Vetter 
> > > > > > > > >>> Cc: Ville Syrjala 
> > > > > > > > >>> Signed-off-by: Manasi Navare 
> > > > > > > > >>> ---
> > > > > > > > >>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > >>>  1 file changed, 7 insertions(+)
> > > > > > > > >>>
> > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> @@ -519,6 +519,13 @@ static int 
> > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > > > > >>>connector_state);
> > > > > > > > >>> if (ret)
> > > > > > > > >>> return ret;
> > > > > > > > >>> +
> > > > > > > > >>> +   if (connector->state->crtc) {
> > > > > > > > >>> +   crtc_state = 
> > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > >>> +   
> > > > > > > > >>> connector->state->crtc);
> > > > > > > > >>> +   if (connector->link_status == 
> > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > >>> +   crtc_state->connectors_changed 
> > > > > > > > >>> = true;
> > > > > > > > >>> +   }
> > > > > > > > >>> }
> > > > > > > > >>>  
> > > > > > > > >>> /*
> > > > > > > > >> This will cause ordinary atomic commits that happen to 
> > > > > > > > >> change connector flags to potentially fail with -EINVAL if 
> > > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > > >> For this reason I'm not sure this flag should be set 
> > > > > > > > >> automatically by the kernel. Could we add add a retrain link 
> > > > > > > > >> property instead, that
> > > > > > > > >> always return 0 when queried, but writing a 1 causing 
> > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > Or just check for allow_modeset before setting 
> > > > > > > > > connectors_changed=true here?
> > > > > > > > 
> > > > > > > > I don't think modesets should be done automatically like that, 
> > > > > > > > even if ALLOW_MODESET is set a modeset may not be expected by 
> > > > > > > > userspace.
> > > > > > > 
> > > > > > > Presumably userspace would want a picture on the screen using any 
> > > > > > > means
> > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it 
> > > > > > > should
> > > > > > > probably say as much?
> > > > > > 
> > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have to 
> > > > > > do a
> > > > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > > > screen.
> > > > > > 
> > > > > > What we could try is to gate this of whether userspace touches the 
> > > > > > mode
> > > > > > property on the corresponding CRTC. I.e. if that's touched (even if 
> > > > > > it's
> > > > > > the same mode), and a link is bad in one of the connectors in the 
> > > > > > state
> > > > > > then we do a full modeset to recover.
> > > > > > 
> > > > > > Another option would be to make the link status writeable. Trying to
> > > > > > change it from bad->good would force the modeset. That would be 
> > > > > > 100% clear
> > > > > > to userspace, not special hacks needed with checking for 
> > > > > > allow_modeset,
> > > > > > no magic property that auto-changes its value. And 100% backwards 
> > > > > > compat
> > > > > > because existing userspace should never touch properties it 

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 05:35:20PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.

Thanks for fixing this.  I noticed a couple of mistakes while reading
through the patch:

> diff --git a/include/drm/drm_modeset_helper_vtables.h 
> b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..e96d662ea572 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
>* to implement blocking and nonblocking commits easily. It is not used
>* by the atomic helpers
>*
> -  * This hook should first commit the given atomic state to the hardware.
> -  * But drivers can add more waiting calls at the start of their
> -  * implementation, e.g. to wait for driver-internal request for implicit
> -  * syncing, before starting to commit the update to the hardware.
> +  * This function is called when the new atomic state has already been
> +  * swapped into the various state pointers. The the passed in state 
> therefore contains

"The the".  It also goes way over 80 columns.

> +  * copies of the old/previous state. This hook should commit the new
> +  * state into hardware. Note that the helpers have already waited for
> +  * preceedning atomic commits and fences, but drivers can add more

"preceeding"

> +  * waiting calls at the start of their implementation, e.g. to wait for
> +  * driver-internal request for implicit syncing, before starting to
> +  * commit the update to the hardware.
>*
>* After the atomic update is committed to the hardware this hook needs
>* to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> -- 
> 2.10.2
> 

-- 
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.

[PATCH 37/37] drm/i915: Implement .get_format_info() hook for CCS

[Ville Syrjälä]

On Fri, Nov 18, 2016 at 03:31:48PM -0800, Ben Widawsky wrote:
> On 16-11-18 21:53:13, Ville Syrjälä wrote:
> >From: Ville Syrjälä 
> >
> >By providing our own format information for the CCS formats, we should
> >be able to make framebuffer_check() do the right thing for the CCS
> >surface as well.
> >
> 
> I was hoping to see that patch as well :-). If you're adding the new fb
> modifiers, I think it'd make sense to make it part of this series.
> Alternatively, I can take 36, and 37 and make it part of my series, then
> integrate that last bit. It's up to you.
> 
> >Note that we'll return the same format info for both Y and Yf tiled
> >format as that's what happens with the non-CCS Y vs. Yf as well. If
> >desired, we could potentially return a unique pointer for each
> >pixel_format+tiling+ccs combination, in which case we immediately be
> >able to tell if any of that stuff changed by just comparing the
> >pointers. But that does sound a bit wasteful space wise.
> >
> >Cc: Ben Widawsky 
> >Cc: intel-gfx at lists.freedesktop.org
> >Signed-off-by: Ville Syrjälä 
> 
> I have a comment below however, you can consider it:
> Reviewed-by: Ben Widawsky 
> 
> >---
> > drivers/gpu/drm/i915/intel_display.c | 37 
> > 
> > include/uapi/drm/drm_fourcc.h|  3 +++
> > 2 files changed, 40 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c 
> >b/drivers/gpu/drm/i915/intel_display.c
> >index 7b7135be3b9e..de6909770c68 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -2488,6 +2488,42 @@ static unsigned int 
> >intel_fb_modifier_to_tiling(uint64_t fb_modifier)
> > }
> > }
> >
> >+static const struct drm_format_info ccs_formats[] = {
> >+{ .format = DRM_FORMAT_XRGB, .depth = 24, .num_planes = 2, .cpp = { 
> >4, 1, }, .hsub = 16, .vsub = 8, },
> >+{ .format = DRM_FORMAT_XBGR, .depth = 24, .num_planes = 2, .cpp = { 
> >4, 1, }, .hsub = 16, .vsub = 8, },
> >+{ .format = DRM_FORMAT_ARGB, .depth = 32, .num_planes = 2, .cpp = { 
> >4, 1, }, .hsub = 16, .vsub = 8, },
> >+{ .format = DRM_FORMAT_ABGR, .depth = 32, .num_planes = 2, .cpp = { 
> >4, 1, }, .hsub = 16, .vsub = 8, },
> >+};
> >+
> >+static const struct drm_format_info *
> >+lookup_format_info(const struct drm_format_info formats[],
> >+   int num_formats, u32 format)
> >+{
> >+int i;
> >+
> >+for (i = 0; i < num_formats; i++) {
> >+if (formats[i].format == format)
> >+return [i];
> >+}
> >+
> >+return NULL;
> >+}
> >+
> >+static const struct drm_format_info *
> >+intel_get_format_info(struct drm_device *dev,
> >+  const struct drm_mode_fb_cmd2 *cmd)
> >+{
> >+switch (cmd->modifier[0]) {
> >+case I915_FORMAT_MOD_Y_TILED_CCS:
> >+case I915_FORMAT_MOD_Yf_TILED_CCS:
> >+return lookup_format_info(ccs_formats,
> >+  ARRAY_SIZE(ccs_formats),
> >+  cmd->pixel_format);
> >+default:
> >+return NULL;
> >+}
> >+}
> >+
> 
> It sort of seems like somewhat of a waste to provide this if implementations 
> are
> allowed to return NULL. It's like saying, "DRM core will check stuff for you 
> if
> you provide the info, but you don't have to do it if you don't want to."

The core will check the stuff anyway. The NULL just means "I don't have
any special requirements, so the core format info will be sufficient".

> If
> that's the case you may as well provide a driver hook to just do the check, 
> ie.
> s/mod_funcs->get_format_info/mode_functs->check_format/

Drivers already have to do a bunch of checks in .fb_create(). In
addition the core does some basic sanity checks before the driver
even sees the mode_cmd (except for the new .get_format_info() hook
that is). I don't want every driver to have to duplicate all of these
basic sanity checks.

One alternative to this scheme would be have a helper function that
every driver would call in .fb_create() that would do these basic sanity
checks. That way we wouldn't need the extra hook, with a slight risk
that driver would forget to call the helper.

> 
> > static int
> > intel_fill_fb_info(struct drm_i915_private *dev_priv,
> >struct drm_framebuffer *fb)
> >@@ -15922,6 +15958,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
> >
> > static const struct drm_mode_config_funcs intel_mode_funcs = {
> > .fb_create = intel_user_framebuffer_create,
> >+.get_format_info = intel_get_format_info,
> > .output_poll_changed = intel_fbdev_output_poll_changed,
> > .atomic_check = intel_atomic_check,
> > .atomic_commit = intel_atomic_commit,
> >diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> >index a5890bf44c0a..2926d916f199 100644
> >--- a/include/uapi/drm/drm_fourcc.h
> >+++ b/include/uapi/drm/drm_fourcc.h
> >@@ -218,6 +218,9 @@ extern "C" {
> > 

[RFC][PATCH 3/3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

[John Stultz]

From: Archit Taneja 

On some adv7511 implementations, we can get some spurious
disconnect signals which can cause monitor probing to fail.

This patch enables HPD (hot plug detect) interrupt support
which allows the monitor to be properly re-initialized when
the spurious disconnect signal goes away.

This also enables proper hotplug support.

Cc: David Airlie 
Cc: Archit Taneja 
Cc: Wolfram Sang 
Cc: Lars-Peter Clausen 
Cc: Laurent Pinchart 
Cc: dri-devel at lists.freedesktop.org
Originally-by: Archit Taneja 
[jstultz: Added proper commit message]
Signed-off-by: John Stultz 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2114a4c..889cf36 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -338,7 +338,7 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
 * Still, let's be safe and stick to the documentation.
 */
regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-ADV7511_INT0_EDID_READY);
+ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
 ADV7511_INT1_DDC_ERROR);
}
@@ -825,6 +825,10 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
if (adv->type == ADV7533)
ret = adv7533_attach_dsi(adv);

+   if (adv->i2c_main->irq)
+   regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
+   ADV7511_INT0_HPD);
+
return ret;
 }

-- 
2.7.4

[RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

Secton 4.1 of the adv7511 programming guide advises one waits
200ms after powering on the chip before trying to communicate
with it via i2c. Not doing so can cause reliability issues when
probing the EDID.

See:
http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf

So this patch simply adds a 200ms sleep at the end of the
power_on path. This greatly improves EDID probing reliabilty
on hotplug with the HiKey device.

Cc: David Airlie 
Cc: Archit Taneja 
Cc: Wolfram Sang 
Cc: Lars-Peter Clausen 
Cc: Laurent Pinchart 
Cc: dri-devel at lists.freedesktop.org
Signed-off-by: John Stultz 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index b240e05..2114a4c 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
 */
regcache_sync(adv7511->regmap);

+   msleep(200);
+
if (adv7511->type == ADV7533)
adv7533_dsi_power_on(adv7511);
 }
-- 
2.7.4

[RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

In chasing down issues with EDID probing, I found some
duplicated but incomplete logic used to power the chip on and
off.

This patch refactors the adv7511_power_on/off functions, so
they can be used for internal needs, and replaces duplicative
logic that powers the chip on and off around the EDID probing
with the common logic.

Cc: David Airlie 
Cc: Archit Taneja 
Cc: Wolfram Sang 
Cc: Lars-Peter Clausen 
Cc: Laurent Pinchart 
Cc: dri-devel at lists.freedesktop.org
Signed-off-by: John Stultz 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +---
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8dba729..b240e05 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }

-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
adv7511->current_edid_segment = -1;

@@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 ADV7511_INT1_DDC_ERROR);
}

+
/*
 * Per spec it is allowed to pulse the HPD signal to indicate that the
 * EDID information has changed. Some monitors do this when they wakeup
@@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)

if (adv7511->type == ADV7533)
adv7533_dsi_power_on(adv7511);
+}

+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+   __adv7511_power_on(adv7511);
adv7511->powered = true;
 }

-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
/* TODO: setup additional power down modes */
regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
@@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)

if (adv7511->type == ADV7533)
adv7533_dsi_power_off(adv7511);
+}

+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+   __adv7511_power_off(adv7511);
adv7511->powered = false;
 }

@@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
unsigned int count;

/* Reading the EDID only works if the device is powered */
-   if (!adv7511->powered) {
-   regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-  ADV7511_POWER_POWER_DOWN, 0);
-   if (adv7511->i2c_main->irq) {
-   regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-ADV7511_INT0_EDID_READY);
-   regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
-ADV7511_INT1_DDC_ERROR);
-   }
-   adv7511->current_edid_segment = -1;
-   }
+   if (!adv7511->powered)
+   __adv7511_power_on(adv7511);

edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);

if (!adv7511->powered)
-   regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-  ADV7511_POWER_POWER_DOWN,
-  ADV7511_POWER_POWER_DOWN);
+   __adv7511_power_off(adv7511);

kfree(adv7511->edid);
adv7511->edid = edid;
-- 
2.7.4

[RFC][PATCH 0/3] adv7511 EDID probing improvements

[John Stultz]

I had been seeing some EDID probing issues with the adv7511 driver
on HiKey recently. After talking with Archit and spending some time
reading the programming guide, I put together the following patch
set which seems to resolve the EDID probing issues.

I wanted to send these out for some early review and feedback

thanks
-john

Cc: David Airlie 
Cc: Archit Taneja 
Cc: Wolfram Sang 
Cc: Lars-Peter Clausen 
Cc: Laurent Pinchart 
Cc: dri-devel at lists.freedesktop.org

Archit Taneja (1):
  drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
improve monitor detection

John Stultz (2):
  drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
reused internally
  drm/bridge: adv7511: Add 200ms delay on power-on

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 38 +++-
 1 file changed, 21 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Ville Syrjälä]

On Mon, Nov 21, 2016 at 03:42:34PM +0200, Laurent Pinchart wrote:
> Hi Ville,
> 
> On Monday 21 Nov 2016 15:31:57 Ville Syrjälä wrote:
> > On Mon, Nov 21, 2016 at 03:23:19PM +0200, Laurent Pinchart wrote:
> > > On Monday 21 Nov 2016 15:18:23 Ville Syrjälä wrote:
> > >> On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> > >>> On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
> >  From: Ville Syrjälä 
> >  
> >  Allow drivers to return a custom drm_format_info structure for
> >  special fb layouts. We'll use this for the compression control surface
> >  in i915.
> >  
> >  Cc: Ben Widawsky 
> >  Cc: intel-gfx at lists.freedesktop.org
> >  Signed-off-by: Ville Syrjälä 
> >  ---
> >  
> >   drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
> >   drivers/gpu/drm/drm_fourcc.c | 25 +
> >   drivers/gpu/drm/drm_framebuffer.c|  9 +++--
> >   drivers/gpu/drm/drm_modeset_helper.c |  2 +-
> >   include/drm/drm_fourcc.h |  6 ++
> >   include/drm/drm_mode_config.h| 15 +++
> >   6 files changed, 55 insertions(+), 4 deletions(-)
> 
> [snip]
> 
> >  diff --git a/drivers/gpu/drm/drm_fourcc.c
> >  b/drivers/gpu/drm/drm_fourcc.c
> >  index 90d2cc8da8eb..7cfaee689f0c 100644
> >  --- a/drivers/gpu/drm/drm_fourcc.c
> >  +++ b/drivers/gpu/drm/drm_fourcc.c
> >  @@ -199,6 +199,31 @@ const struct drm_format_info
> >  *drm_format_info(u32 format)
> >   EXPORT_SYMBOL(drm_format_info);
> >   
> >   /**
> >  + * drm_format_info - query information for a given framebuffer
> >  configuration
> > >>> 
> > >>> I assume you meant drm_get_format_info()
> > >> 
> > >> Yes.
> > >> 
> >  + * @dev: DRM device
> > >>> 
> > >>> Do we need the dev pointer ?
> > >> 
> > >> Not at the moment. I was thinking we might allow drivers to return a
> > >> different set of formats based on the device type, but I'm not sure
> > >> that's all that useful since drivers will have to check for unsupported
> > >> formats anyway in .fb_create(). The only use case might be if you need
> > >> to select between two different format info structs based on the device
> > >> type, because you simply can't tell the formats apart based on the
> > >> mode_cmd. But that sort of thing feels like a bad idea to me, and might
> > >> as well just require that you must be able to tell formats that require
> > >> different format intos apart based on the mode_cmd (eg. by having
> > >> different modifiers on them).
> > >> 
> > >> So I guess we could just drop the 'dev' argument to make it harder for
> > >> people to make that sort of mistake.
> > > 
> > > I think that's a good idea, yes.
> > > 
> >  + * @mode_cmd: metadata from the userspace fb creation request
> >  + *
> >  + * Returns:
> >  + * The instance of struct drm_format_info that describes the pixel
> >  format, or
> >  + * NULL if the format is unsupported.
> > >>> 
> > >>> It would be useful to document how this function differs from
> > >>> drm_format_info(). I also wonder whether it would make sense to
> > >>> completely replace drm_format_info() to avoid keeping two separate but
> > >>> very similar functions.
> > >> 
> > >> Yeah, that is basically what I was thinking. But I didn't feel like
> > >> doing that myself as it looked like that might involve actual work
> > >> in some of the drivers. I figured I'd leave it up to whoever cares
> > >> about said drivers.
> > > 
> > > Which driver(s) are you thinking about ?
> > 
> > The ones that my cocci stuff couldn't convert over to fb->format.
> 
> How about at least making drm_get_format_info() the default but converting 
> what can be converted with coccinelle, and marking drm_format_info() as 
> deprecated ?

I think I already did everything except the "mark as deprecated" part.
And adding that last bit into the patch would be trivial.

> 
> > > If we want to make drm_get_format_info() the default we obviously need to
> > > pass modifiers directly, as in most cases we won't have a struct
> > > drm_mode_fb_cmd2 to pass to the function. If we remove the dev argument
> > > you could just pass NULL modifiers in most cases, I don't think that would
> > > involve much rework in drivers.
> >
> > fb->format is probably the right choice in most cases. But some drivers
> > seemed to have some kind of internal format info struct instead which
> > was in the way of doing a trivial conversion. I didn't want to start
> > doing non-trivial conversions since the series was already way too big
> > as is.
> 
> That's an interesting point I wanted to also mention. We have drivers that 
> include formats information tables duplicating the one in the DRM core, with 
> additional driver-specific information (see rcar_du_format_info() in 
> drivers/gpu/drm/rcar-du/rcar_du_kms.c for instance). I wonder whether it 
> would 
> be 

[PATCH libdrm v2] intel: Add a getter for the intel_context ctx_id

[Robert Bragg]

Renamed to avoid the seemingly redundant 'context_' infix and note that it's
been reviewed by Matthew Auld.

--- >8 ---

Exposing the u32 context ID makes it possible to define new drm kernel
interfaces based on the same IDs that e.g. execbuf uses to identify a
gem context, that aren't themselves abstracted by libdrm but need to be
used by libdrm/drm_intel_context based clients such as (parts of) i-g-t
or Mesa.

For example this can be used to configure an i915-perf stream to collect
metrics for a specific context.

v2: s/drm_intel_gem_context_get_context_id/drm_intel_gem_context_get_id/

Signed-off: Robert Bragg 
Reviewed-by: Matthew Auld 
---
 intel/intel_bufmgr.h |  2 ++
 intel/intel_bufmgr_gem.c | 11 +++
 2 files changed, 13 insertions(+)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index ce4e70d..85e4ff7 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -212,6 +212,8 @@ int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr 
*bufmgr);
 int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);

 drm_intel_context *drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr);
+int drm_intel_gem_context_get_id(drm_intel_context *ctx,
+ uint32_t *ctx_id);
 void drm_intel_gem_context_destroy(drm_intel_context *ctx);
 int drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
  int used, unsigned int flags);
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 15c79b3..5fc022a 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3184,6 +3184,17 @@ drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr)
return context;
 }

+int
+drm_intel_gem_context_get_id(drm_intel_context *ctx, uint32_t *ctx_id)
+{
+   if (ctx == NULL)
+   return -EINVAL;
+
+   *ctx_id = ctx->ctx_id;
+
+   return 0;
+}
+
 void
 drm_intel_gem_context_destroy(drm_intel_context *ctx)
 {
-- 
2.10.1

[PATCH v5 2/2] drm/panel: simple: add support for Sharp LQ150X1LG11 panels

[Peter Rosin]

From: Gustaf Lindström 

The Sharp 15" LQ150X1LG11 panel is an XGA TFT LCD panel.

The simple-panel driver is used to get support for essential
functionality of the panel.

Signed-off-by: Gustaf Lindström 
Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/panel/panel-simple.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 113db3c4a633..76f0ef7e5b7c 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1420,6 +1420,30 @@ static const struct panel_desc sharp_lq123p1jx31 = {
},
 };

+static const struct drm_display_mode sharp_lq150x1lg11_mode = {
+   .clock = 71100,
+   .hdisplay = 1024,
+   .hsync_start = 1024 + 168,
+   .hsync_end = 1024 + 168 + 64,
+   .htotal = 1024 + 168 + 64 + 88,
+   .vdisplay = 768,
+   .vsync_start = 768 + 37,
+   .vsync_end = 768 + 37 + 2,
+   .vtotal = 768 + 37 + 2 + 8,
+   .vrefresh = 60,
+};
+
+static const struct panel_desc sharp_lq150x1lg11 = {
+   .modes = _lq150x1lg11_mode,
+   .num_modes = 1,
+   .bpc = 6,
+   .size = {
+   .width = 304,
+   .height = 228,
+   },
+   .bus_format = MEDIA_BUS_FMT_RGB565_1X16,
+};
+
 static const struct drm_display_mode shelly_sca07010_bfn_lnn_mode = {
.clock = 33300,
.hdisplay = 800,
@@ -1683,6 +1707,9 @@ static const struct of_device_id platform_of_match[] = {
.compatible = "sharp,lq123p1jx31",
.data = _lq123p1jx31,
}, {
+   .compatible = "sharp,lq150x1lg11",
+   .data = _lq150x1lg11,
+   }, {
.compatible = "shelly,sca07010-bfn-lnn",
.data = _sca07010_bfn_lnn,
}, {
-- 
2.1.4

[PATCH v5 1/2] dt-bindings: display: Add Sharp LQ150X1LG11 panel binding

[Peter Rosin]

The Sharp 15" LQ150X1LG11 panel is an XGA TFT LCD panel.

Signed-off-by: Peter Rosin 
---
 .../bindings/display/panel/sharp,lq150x1lg11.txt   | 36 ++
 1 file changed, 36 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt

diff --git 
a/Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt 
b/Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt
new file mode 100644
index ..0f57c3143506
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt
@@ -0,0 +1,36 @@
+Sharp 15" LQ150X1LG11 XGA TFT LCD panel
+
+Required properties:
+- compatible: should be "sharp,lq150x1lg11"
+- power-supply: regulator to provide the VCC supply voltage (3.3 volts)
+
+Optional properties:
+- backlight: phandle of the backlight device
+- rlud-gpios: a single GPIO for the RL/UD (rotate 180 degrees) pin.
+- sellvds-gpios: a single GPIO for the SELLVDS pin.
+
+If rlud-gpios and/or sellvds-gpios are not specified, the RL/UD and/or SELLVDS
+pins are assumed to be handled appropriately by the hardware.
+
+Example:
+
+   backlight: backlight {
+   compatible = "pwm-backlight";
+   pwms = < 0 10>;  /* VBR */
+
+   brightness-levels = <0 20 40 60 80 100>;
+   default-brightness-level = <2>;
+
+   power-supply = <_12v_reg>;   /* VDD */
+   enable-gpios = < 42 GPIO_ACTIVE_HIGH>;  /* XSTABY */
+   };
+
+   panel {
+   compatible = "sharp,lq150x1lg11";
+
+   power-supply = <_3v3_reg>;   /* VCC */
+
+   backlight = <>;
+   rlud-gpios = < 17 GPIO_ACTIVE_HIGH>;/* RL/UD */
+   sellvds-gpios = < 18 GPIO_ACTIVE_HIGH>; /* SELLVDS */
+   };
-- 
2.1.4

[PATCH v5 0/2] drm/panel: simple: add support for Sharp LQ150X1LG11 panels

[Peter Rosin]

Hi!

This patch seems to have been forgotten? Thierry said that a
resend was not needed, but time is passing and the merge window
is nearing, so I'm resending anyway with the squashed .bpc-fix.

v4 -> v5 changes:
- change sharp_lq150x1lg11.bpc to 6 as noted by Thierry
- rebased onto v4.9-rc6

v3 -> v4 changes:
- addressed review comments from Rob (lvds -> sellvds and a couple of typos).

Cheers,
Peter

Gustaf Lindström (1):
  drm/panel: simple: add support for Sharp LQ150X1LG11 panels

Peter Rosin (1):
  dt-bindings: display: Add Sharp LQ150X1LG11 panel binding

 .../bindings/display/panel/sharp,lq150x1lg11.txt   | 36 ++
 drivers/gpu/drm/panel/panel-simple.c   | 27 
 2 files changed, 63 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt

-- 
2.1.4

[PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Laurent Pinchart]

Hi Ville,

On Monday 21 Nov 2016 15:31:57 Ville Syrjälä wrote:
> On Mon, Nov 21, 2016 at 03:23:19PM +0200, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 15:18:23 Ville Syrjälä wrote:
> >> On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> >>> On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
>  From: Ville Syrjälä 
>  
>  Allow drivers to return a custom drm_format_info structure for
>  special fb layouts. We'll use this for the compression control surface
>  in i915.
>  
>  Cc: Ben Widawsky 
>  Cc: intel-gfx at lists.freedesktop.org
>  Signed-off-by: Ville Syrjälä 
>  ---
>  
>   drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
>   drivers/gpu/drm/drm_fourcc.c | 25 +
>   drivers/gpu/drm/drm_framebuffer.c|  9 +++--
>   drivers/gpu/drm/drm_modeset_helper.c |  2 +-
>   include/drm/drm_fourcc.h |  6 ++
>   include/drm/drm_mode_config.h| 15 +++
>   6 files changed, 55 insertions(+), 4 deletions(-)

[snip]

>  diff --git a/drivers/gpu/drm/drm_fourcc.c
>  b/drivers/gpu/drm/drm_fourcc.c
>  index 90d2cc8da8eb..7cfaee689f0c 100644
>  --- a/drivers/gpu/drm/drm_fourcc.c
>  +++ b/drivers/gpu/drm/drm_fourcc.c
>  @@ -199,6 +199,31 @@ const struct drm_format_info
>  *drm_format_info(u32 format)
>   EXPORT_SYMBOL(drm_format_info);
>   
>   /**
>  + * drm_format_info - query information for a given framebuffer
>  configuration
> >>> 
> >>> I assume you meant drm_get_format_info()
> >> 
> >> Yes.
> >> 
>  + * @dev: DRM device
> >>> 
> >>> Do we need the dev pointer ?
> >> 
> >> Not at the moment. I was thinking we might allow drivers to return a
> >> different set of formats based on the device type, but I'm not sure
> >> that's all that useful since drivers will have to check for unsupported
> >> formats anyway in .fb_create(). The only use case might be if you need
> >> to select between two different format info structs based on the device
> >> type, because you simply can't tell the formats apart based on the
> >> mode_cmd. But that sort of thing feels like a bad idea to me, and might
> >> as well just require that you must be able to tell formats that require
> >> different format intos apart based on the mode_cmd (eg. by having
> >> different modifiers on them).
> >> 
> >> So I guess we could just drop the 'dev' argument to make it harder for
> >> people to make that sort of mistake.
> > 
> > I think that's a good idea, yes.
> > 
>  + * @mode_cmd: metadata from the userspace fb creation request
>  + *
>  + * Returns:
>  + * The instance of struct drm_format_info that describes the pixel
>  format, or
>  + * NULL if the format is unsupported.
> >>> 
> >>> It would be useful to document how this function differs from
> >>> drm_format_info(). I also wonder whether it would make sense to
> >>> completely replace drm_format_info() to avoid keeping two separate but
> >>> very similar functions.
> >> 
> >> Yeah, that is basically what I was thinking. But I didn't feel like
> >> doing that myself as it looked like that might involve actual work
> >> in some of the drivers. I figured I'd leave it up to whoever cares
> >> about said drivers.
> > 
> > Which driver(s) are you thinking about ?
> 
> The ones that my cocci stuff couldn't convert over to fb->format.

How about at least making drm_get_format_info() the default but converting 
what can be converted with coccinelle, and marking drm_format_info() as 
deprecated ?

> > If we want to make drm_get_format_info() the default we obviously need to
> > pass modifiers directly, as in most cases we won't have a struct
> > drm_mode_fb_cmd2 to pass to the function. If we remove the dev argument
> > you could just pass NULL modifiers in most cases, I don't think that would
> > involve much rework in drivers.
>
> fb->format is probably the right choice in most cases. But some drivers
> seemed to have some kind of internal format info struct instead which
> was in the way of doing a trivial conversion. I didn't want to start
> doing non-trivial conversions since the series was already way too big
> as is.

That's an interesting point I wanted to also mention. We have drivers that 
include formats information tables duplicating the one in the DRM core, with 
additional driver-specific information (see rcar_du_format_info() in 
drivers/gpu/drm/rcar-du/rcar_du_kms.c for instance). I wonder whether it would 
be possible to come up with a simple API that would allow providing those 
driver-specific data to the core, and get them back from the 
drm_get_format_info() function.

-- 
Regards,

Laurent Pinchart

[PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Ville Syrjälä]

On Mon, Nov 21, 2016 at 03:23:19PM +0200, Laurent Pinchart wrote:
> Hi Ville,
> 
> On Monday 21 Nov 2016 15:18:23 Ville Syrjälä wrote:
> > On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> > > On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
> > >> From: Ville Syrjälä 
> > >> 
> > >> Allow drivers to return a custom drm_format_info structure for special
> > >> fb layouts. We'll use this for the compression control surface in i915.
> > >> 
> > >> Cc: Ben Widawsky 
> > >> Cc: intel-gfx at lists.freedesktop.org
> > >> Signed-off-by: Ville Syrjälä 
> > >> ---
> > >> 
> > >>  drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
> > >>  drivers/gpu/drm/drm_fourcc.c | 25 +
> > >>  drivers/gpu/drm/drm_framebuffer.c|  9 +++--
> > >>  drivers/gpu/drm/drm_modeset_helper.c |  2 +-
> > >>  include/drm/drm_fourcc.h |  6 ++
> > >>  include/drm/drm_mode_config.h| 15 +++
> > >>  6 files changed, 55 insertions(+), 4 deletions(-)
> > >> 
> > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..d7f8876cf5e9
> > >> 100644
> > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> @@ -186,7 +186,7 @@ struct drm_framebuffer
> > >> *drm_fb_cma_create_with_funcs(struct drm_device *dev, int ret;
> > >> 
> > >>  int i;
> > >> 
> > >> -info = drm_format_info(mode_cmd->pixel_format);
> > >> +info = drm_get_format_info(dev, mode_cmd);
> > >>  if (!info)
> > >>  return ERR_PTR(-EINVAL);
> > >> 
> > >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > >> index 90d2cc8da8eb..7cfaee689f0c 100644
> > >> --- a/drivers/gpu/drm/drm_fourcc.c
> > >> +++ b/drivers/gpu/drm/drm_fourcc.c
> > >> @@ -199,6 +199,31 @@ const struct drm_format_info *drm_format_info(u32
> > >> format)
> > >>  EXPORT_SYMBOL(drm_format_info);
> > >> 
> > >>  /**
> > >> + * drm_format_info - query information for a given framebuffer
> > >> configuration
> > > 
> > > I assume you meant drm_get_format_info()
> > 
> > Yes.
> > 
> > >> + * @dev: DRM device
> > > 
> > > Do we need the dev pointer ?
> > 
> > Not at the moment. I was thinking we might allow drivers to return a
> > different set of formats based on the device type, but I'm not sure
> > that's all that useful since drivers will have to check for unsupported
> > formats anyway in .fb_create(). The only use case might be if you need
> > to select between two different format info structs based on the device
> > type, because you simply can't tell the formats apart based on the
> > mode_cmd. But that sort of thing feels like a bad idea to me, and might
> > as well just require that you must be able to tell formats that require
> > different format intos apart based on the mode_cmd (eg. by having
> > different modifiers on them).
> > 
> > So I guess we could just drop the 'dev' argument to make it harder for
> > people to make that sort of mistake.
> 
> I think that's a good idea, yes.
> 
> > >> + * @mode_cmd: metadata from the userspace fb creation request
> > >> + *
> > >> + * Returns:
> > >> + * The instance of struct drm_format_info that describes the pixel
> > >> format, or
> > >> + * NULL if the format is unsupported.
> > > 
> > > It would be useful to document how this function differs from
> > > drm_format_info(). I also wonder whether it would make sense to completely
> > > replace drm_format_info() to avoid keeping two separate but very similar
> > > functions.
> > 
> > Yeah, that is basically what I was thinking. But I didn't feel like
> > doing that myself as it looked like that might involve actual work
> > in some of the drivers. I figured I'd leave it up to whoever cares
> > about said drivers.
> 
> Which driver(s) are you thinking about ?

The ones that my cocci stuff couldn't convert over to fb->format.

> If we want to make 
> drm_get_format_info() the default we obviously need to pass modifiers 
> directly, as in most cases we won't have a struct drm_mode_fb_cmd2 to pass to 
> the function. If we remove the dev argument you could just pass NULL 
> modifiers 
> in most cases, I don't think that would involve much rework in drivers.

fb->format is probably the right choice in most cases. But some drivers
seemed to have some kind of internal format info struct instead which
was in the way of doing a trivial conversion. I didn't want to start
doing non-trivial conversions since the series was already way too big
as is.

> 
> > >> + */
> > >> +const struct drm_format_info *
> > >> +drm_get_format_info(struct drm_device *dev,
> > >> +const struct drm_mode_fb_cmd2 *mode_cmd)
> > >> +{
> > >> +const struct drm_format_info *info = NULL;
> > >> +
> > >> +if (dev->mode_config.funcs->get_format_info)
> > >> +info = dev->mode_config.funcs->get_format_info(dev, 
> > >> 

[PATCH 08/37] drm/arcpgu: Add local 'fb' variables

[Alexey Brodkin]

Hi Ville,

On Fri, 2016-11-18 at 21:52 +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Add a local 'fb' variable to a few places to get rid of the
> 'crtc->primary->fb' stuff. Looks neater and helps me with my ppor
> coccinelle skills later.
> 
> Cc: Alexey Brodkin 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
> b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index 7130b044b004..5c26c5f126a3 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -35,7 +35,8 @@ static struct simplefb_format supported_formats[] = {
>  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>  {
>  struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> - uint32_t pixel_format = crtc->primary->state->fb->pixel_format;
> + const struct drm_framebuffer *fb = crtc->primary->state->fb;
> + uint32_t pixel_format = fb->pixel_format;
>  struct simplefb_format *format = NULL;
>  int i;

Acked-by: Alexey Brodkin 

[Intel-gfx] [PATCH 37/37] drm/i915: Implement .get_format_info() hook for CCS

[Ville Syrjälä]

On Mon, Nov 21, 2016 at 08:42:13AM +, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 18/11/2016 19:53, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > By providing our own format information for the CCS formats, we should
> > be able to make framebuffer_check() do the right thing for the CCS
> > surface as well.
> >
> > Note that we'll return the same format info for both Y and Yf tiled
> > format as that's what happens with the non-CCS Y vs. Yf as well. If
> > desired, we could potentially return a unique pointer for each
> > pixel_format+tiling+ccs combination, in which case we immediately be
> > able to tell if any of that stuff changed by just comparing the
> > pointers. But that does sound a bit wasteful space wise.
> >
> > Cc: Ben Widawsky 
> > Cc: intel-gfx at lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 37 
> > 
> >  include/uapi/drm/drm_fourcc.h|  3 +++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7b7135be3b9e..de6909770c68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2488,6 +2488,42 @@ static unsigned int 
> > intel_fb_modifier_to_tiling(uint64_t fb_modifier)
> > }
> >  }
> >
> > +static const struct drm_format_info ccs_formats[] = {
> > +   { .format = DRM_FORMAT_XRGB, .depth = 24, .num_planes = 2, .cpp = { 
> > 4, 1, }, .hsub = 16, .vsub = 8, },
> > +   { .format = DRM_FORMAT_XBGR, .depth = 24, .num_planes = 2, .cpp = { 
> > 4, 1, }, .hsub = 16, .vsub = 8, },
> > +   { .format = DRM_FORMAT_ARGB, .depth = 32, .num_planes = 2, .cpp = { 
> > 4, 1, }, .hsub = 16, .vsub = 8, },
> > +   { .format = DRM_FORMAT_ABGR, .depth = 32, .num_planes = 2, .cpp = { 
> > 4, 1, }, .hsub = 16, .vsub = 8, },
> > +};
> > +
> > +static const struct drm_format_info *
> > +lookup_format_info(const struct drm_format_info formats[],
> > +  int num_formats, u32 format)
> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < num_formats; i++) {
> > +   if (formats[i].format == format)
> > +   return [i];
> > +   }
> > +
> > +   return NULL;
> > +}
> > +
> > +static const struct drm_format_info *
> > +intel_get_format_info(struct drm_device *dev,
> > + const struct drm_mode_fb_cmd2 *cmd)
> > +{
> > +   switch (cmd->modifier[0]) {
> > +   case I915_FORMAT_MOD_Y_TILED_CCS:
> > +   case I915_FORMAT_MOD_Yf_TILED_CCS:
> > +   return lookup_format_info(ccs_formats,
> > + ARRAY_SIZE(ccs_formats),
> > + cmd->pixel_format);
> > +   default:
> > +   return NULL;
> > +   }
> > +}
> > +
> >  static int
> >  intel_fill_fb_info(struct drm_i915_private *dev_priv,
> >struct drm_framebuffer *fb)
> > @@ -15922,6 +15958,7 @@ intel_user_framebuffer_create(struct drm_device 
> > *dev,
> >
> >  static const struct drm_mode_config_funcs intel_mode_funcs = {
> > .fb_create = intel_user_framebuffer_create,
> > +   .get_format_info = intel_get_format_info,
> > .output_poll_changed = intel_fbdev_output_poll_changed,
> > .atomic_check = intel_atomic_check,
> > .atomic_commit = intel_atomic_commit,
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index a5890bf44c0a..2926d916f199 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -218,6 +218,9 @@ extern "C" {
> >   */
> >  #define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
> >
> > +#define I915_FORMAT_MOD_Y_TILED_CCSfourcc_mod_code(INTEL, 4)
> > +#define I915_FORMAT_MOD_Yf_TILED_CCS   fourcc_mod_code(INTEL, 5)
> > +
> 
> I think when fb modifiers were started the idea was that we would later 
> partition our vendor bit space for different classes of things and have 
> helper functions to extract the tiling, etc, from them.
> 
> For example have first 3-4 bits represent the tiling, then in this case 
> one bit for CCS, etc.
> 
> Have you considered that when adding these ones, and concluded this 
> different scheme is better for some reason?

I haven't considered anything. And obviously this patch isn't meant
for inclusion as is. I just needed sometime to make it compile.

Generally I don't think adding magic meaning for individual bits for
things like this is a particularly good idea. Every time I've seen a
scheme like that it has eventually turned ugly on account of running
out of bits in one place or another.

-- 
Ville Syrjälä
Intel OTC

[PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Laurent Pinchart]

Hi Ville,

On Monday 21 Nov 2016 15:18:23 Ville Syrjälä wrote:
> On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> > On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
> >> From: Ville Syrjälä 
> >> 
> >> Allow drivers to return a custom drm_format_info structure for special
> >> fb layouts. We'll use this for the compression control surface in i915.
> >> 
> >> Cc: Ben Widawsky 
> >> Cc: intel-gfx at lists.freedesktop.org
> >> Signed-off-by: Ville Syrjälä 
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
> >>  drivers/gpu/drm/drm_fourcc.c | 25 +
> >>  drivers/gpu/drm/drm_framebuffer.c|  9 +++--
> >>  drivers/gpu/drm/drm_modeset_helper.c |  2 +-
> >>  include/drm/drm_fourcc.h |  6 ++
> >>  include/drm/drm_mode_config.h| 15 +++
> >>  6 files changed, 55 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..d7f8876cf5e9
> >> 100644
> >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> @@ -186,7 +186,7 @@ struct drm_framebuffer
> >> *drm_fb_cma_create_with_funcs(struct drm_device *dev, int ret;
> >> 
> >>int i;
> >> 
> >> -  info = drm_format_info(mode_cmd->pixel_format);
> >> +  info = drm_get_format_info(dev, mode_cmd);
> >>if (!info)
> >>return ERR_PTR(-EINVAL);
> >> 
> >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> >> index 90d2cc8da8eb..7cfaee689f0c 100644
> >> --- a/drivers/gpu/drm/drm_fourcc.c
> >> +++ b/drivers/gpu/drm/drm_fourcc.c
> >> @@ -199,6 +199,31 @@ const struct drm_format_info *drm_format_info(u32
> >> format)
> >>  EXPORT_SYMBOL(drm_format_info);
> >> 
> >>  /**
> >> + * drm_format_info - query information for a given framebuffer
> >> configuration
> > 
> > I assume you meant drm_get_format_info()
> 
> Yes.
> 
> >> + * @dev: DRM device
> > 
> > Do we need the dev pointer ?
> 
> Not at the moment. I was thinking we might allow drivers to return a
> different set of formats based on the device type, but I'm not sure
> that's all that useful since drivers will have to check for unsupported
> formats anyway in .fb_create(). The only use case might be if you need
> to select between two different format info structs based on the device
> type, because you simply can't tell the formats apart based on the
> mode_cmd. But that sort of thing feels like a bad idea to me, and might
> as well just require that you must be able to tell formats that require
> different format intos apart based on the mode_cmd (eg. by having
> different modifiers on them).
> 
> So I guess we could just drop the 'dev' argument to make it harder for
> people to make that sort of mistake.

I think that's a good idea, yes.

> >> + * @mode_cmd: metadata from the userspace fb creation request
> >> + *
> >> + * Returns:
> >> + * The instance of struct drm_format_info that describes the pixel
> >> format, or
> >> + * NULL if the format is unsupported.
> > 
> > It would be useful to document how this function differs from
> > drm_format_info(). I also wonder whether it would make sense to completely
> > replace drm_format_info() to avoid keeping two separate but very similar
> > functions.
> 
> Yeah, that is basically what I was thinking. But I didn't feel like
> doing that myself as it looked like that might involve actual work
> in some of the drivers. I figured I'd leave it up to whoever cares
> about said drivers.

Which driver(s) are you thinking about ? If we want to make 
drm_get_format_info() the default we obviously need to pass modifiers 
directly, as in most cases we won't have a struct drm_mode_fb_cmd2 to pass to 
the function. If we remove the dev argument you could just pass NULL modifiers 
in most cases, I don't think that would involve much rework in drivers.

> >> + */
> >> +const struct drm_format_info *
> >> +drm_get_format_info(struct drm_device *dev,
> >> +  const struct drm_mode_fb_cmd2 *mode_cmd)
> >> +{
> >> +  const struct drm_format_info *info = NULL;
> >> +
> >> +  if (dev->mode_config.funcs->get_format_info)
> >> +  info = dev->mode_config.funcs->get_format_info(dev, mode_cmd);
> >> +
> >> +  if (!info)
> >> +  info = drm_format_info(mode_cmd->pixel_format);
> >> +
> >> +  return info;
> >> +}
> >> +EXPORT_SYMBOL(drm_get_format_info);

-- 
Regards,

Laurent Pinchart

[PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Ville Syrjälä]

On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > Allow drivers to return a custom drm_format_info structure for special
> > fb layouts. We'll use this for the compression control surface in i915.
> > 
> > Cc: Ben Widawsky 
> > Cc: intel-gfx at lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
> >  drivers/gpu/drm/drm_fourcc.c | 25 +
> >  drivers/gpu/drm/drm_framebuffer.c|  9 +++--
> >  drivers/gpu/drm/drm_modeset_helper.c |  2 +-
> >  include/drm/drm_fourcc.h |  6 ++
> >  include/drm/drm_mode_config.h| 15 +++
> >  6 files changed, 55 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..d7f8876cf5e9
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -186,7 +186,7 @@ struct drm_framebuffer
> > *drm_fb_cma_create_with_funcs(struct drm_device *dev, int ret;
> > int i;
> > 
> > -   info = drm_format_info(mode_cmd->pixel_format);
> > +   info = drm_get_format_info(dev, mode_cmd);
> > if (!info)
> > return ERR_PTR(-EINVAL);
> > 
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 90d2cc8da8eb..7cfaee689f0c 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -199,6 +199,31 @@ const struct drm_format_info *drm_format_info(u32
> > format) EXPORT_SYMBOL(drm_format_info);
> > 
> >  /**
> > + * drm_format_info - query information for a given framebuffer
> > configuration
> 
> I assume you meant drm_get_format_info()

Yes.

> 
> > + * @dev: DRM device
> 
> Do we need the dev pointer ?

Not at the moment. I was thinking we might allow drivers to return a
different set of formats based on the device type, but I'm not sure
that's all that useful since drivers will have to check for unsupported
formats anyway in .fb_create(). The only use case might be if you need
to select between two different format info structs based on the device
type, because you simply can't tell the formats apart based on the
mode_cmd. But that sort of thing feels like a bad idea to me, and might
as well just require that you must be able to tell formats that require
different format intos apart based on the mode_cmd (eg. by having
different modifiers on them).

So I guess we could just drop the 'dev' argument to make it harder for
people to make that sort of mistake.

> 
> > + * @mode_cmd: metadata from the userspace fb creation request
> > + *
> > + * Returns:
> > + * The instance of struct drm_format_info that describes the pixel format,
> > or
> > + * NULL if the format is unsupported.
> 
> It would be useful to document how this function differs from 
> drm_format_info(). I also wonder whether it would make sense to completely 
> replace drm_format_info() to avoid keeping two separate but very similar 
> functions.

Yeah, that is basically what I was thinking. But I didn't feel like
doing that myself as it looked like that might involve actual work
in some of the drivers. I figured I'd leave it up to whoever cares
about said drivers.

> 
> > + */
> > +const struct drm_format_info *
> > +drm_get_format_info(struct drm_device *dev,
> > +   const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +   const struct drm_format_info *info = NULL;
> > +
> > +   if (dev->mode_config.funcs->get_format_info)
> > +   info = dev->mode_config.funcs->get_format_info(dev, mode_cmd);
> > +
> > +   if (!info)
> > +   info = drm_format_info(mode_cmd->pixel_format);
> > +
> > +   return info;
> > +}
> > +EXPORT_SYMBOL(drm_get_format_info);
> > +
> > +/**
> >   * drm_format_num_planes - get the number of planes for format
> >   * @format: pixel format (DRM_FORMAT_*)
> >   *
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c index 94ddab41f24f..292930a5dcc2 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -126,11 +126,13 @@ int drm_mode_addfb(struct drm_device *dev,
> > return 0;
> >  }
> > 
> > -static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > +static int framebuffer_check(struct drm_device *dev,
> > +const struct drm_mode_fb_cmd2 *r)
> >  {
> > const struct drm_format_info *info;
> > int i;
> > 
> > +   /* check if the format is supported at all */
> > info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
> > if (!info) {
> > struct drm_format_name_buf format_name;
> > @@ -140,6 +142,9 @@ static int framebuffer_check(const struct
> > drm_mode_fb_cmd2 *r) return -EINVAL;
> > }
> > 
> > +   /* now let the 

[PATCH libdrm] libdrm: random typo fixes

[Alex Deucher]

On Sun, Nov 20, 2016 at 1:25 PM, Grazvydas Ignotas  wrote:
> Just some trivial boring typo fixes all over the tree.
> READMEs and comments only.
>
> Signed-off-by: Grazvydas Ignotas 

Reviewed-by: Alex Deucher 

> ---
>  README|  2 +-
>  include/drm/README|  2 +-
>  intel/intel_bufmgr_fake.c |  4 ++--
>  intel/intel_bufmgr_gem.c  |  6 +++---
>  radeon/radeon_cs_gem.c| 10 +-
>  radeon/radeon_surface.c   |  2 +-
>  6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/README b/README
> index 603a1c1..7eeae38 100644
> --- a/README
> +++ b/README
> @@ -1,7 +1,7 @@
>  libdrm - userspace library for drm
>
>  This  is libdrm,  a userspace  library for  accessing the  DRM, direct
> -rendering  manager, on  Linux,  BSD and  other  operating systes  that
> +rendering  manager, on  Linux,  BSD and  other  operating systems that
>  support the  ioctl interface.  The library  provides wrapper functions
>  for the  ioctls to avoid  exposing the kernel interface  directly, and
>  for chipsets with drm memory manager, support for tracking relocations
> diff --git a/include/drm/README b/include/drm/README
> index c3292f3..a50b02c 100644
> --- a/include/drm/README
> +++ b/include/drm/README
> @@ -89,7 +89,7 @@ Nearly all headers:
>  Status: Trivial.
>
>  Most UMS headers:
> - - Not using fixed size interers - compat ioctls are broken.
> + - Not using fixed size integers - compat ioctls are broken.
>  Status: ?
>  Promote to fixed size ints, which match the current (32bit) ones.
>
> diff --git a/intel/intel_bufmgr_fake.c b/intel/intel_bufmgr_fake.c
> index 24b3732..641df6a 100644
> --- a/intel/intel_bufmgr_fake.c
> +++ b/intel/intel_bufmgr_fake.c
> @@ -737,7 +737,7 @@ drm_intel_bufmgr_fake_wait_idle(drm_intel_bufmgr_fake 
> *bufmgr_fake)
>  /**
>   * Wait for rendering to a buffer to complete.
>   *
> - * It is assumed that the bathcbuffer which performed the rendering included
> + * It is assumed that the batchbuffer which performed the rendering included
>   * the necessary flushing.
>   */
>  static void
> @@ -1200,7 +1200,7 @@ static int
> assert(!(bo_fake->flags & (BM_NO_BACKING_STORE | BM_PINNED)));
>
> /* Actually, should be able to just wait for a fence on the
> -* mmory, hich we would be tracking when we free it.  Waiting
> +* memory, which we would be tracking when we free it. Waiting
>  * for idle is a sufficiently large hammer for now.
>  */
> drm_intel_bufmgr_fake_wait_idle(bufmgr_fake);
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 15c79b3..612b125 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -256,7 +256,7 @@ struct _drm_intel_bo_gem {
>  * Boolean of whether the GPU is definitely not accessing the buffer.
>  *
>  * This is only valid when reusable, since non-reusable
> -* buffers are those that have been shared wth other
> +* buffers are those that have been shared with other
>  * processes, so we don't know their state.
>  */
> bool idle;
> @@ -294,7 +294,7 @@ struct _drm_intel_bo_gem {
>  */
> int reloc_tree_fences;
>
> -   /** Flags that we may need to do the SW_FINSIH ioctl on unmap. */
> +   /** Flags that we may need to do the SW_FINISH ioctl on unmap. */
> bool mapped_cpu_write;
>  };
>
> @@ -1719,7 +1719,7 @@ static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
> }
>
> /* We need to unmap after every innovation as we cannot track
> -* an open vma for every bo as that will exhaasut the system
> +* an open vma for every bo as that will exhaust the system
>  * limits and cause later failures.
>  */
> if (--bo_gem->map_count == 0) {
> diff --git a/radeon/radeon_cs_gem.c b/radeon/radeon_cs_gem.c
> index 23f33af..f3dccb6 100644
> --- a/radeon/radeon_cs_gem.c
> +++ b/radeon/radeon_cs_gem.c
> @@ -189,7 +189,7 @@ static int cs_gem_write_reloc(struct radeon_cs_int *cs,
>  /* check domains */
>  if ((read_domain && write_domain) || (!read_domain && !write_domain)) {
>  /* in one CS a bo can only be in read or write domain but not
> - * in read & write domain at the same sime
> + * in read & write domain at the same time
>   */
>  return -EINVAL;
>  }
> @@ -242,7 +242,7 @@ static int cs_gem_write_reloc(struct radeon_cs_int *cs,
>  }
>  /* new relocation */
>  if (csg->base.crelocs >= csg->nrelocs) {
> -/* allocate more memory (TODO: should use a slab allocatore maybe) */
> +/* allocate more memory (TODO: should use a slab allocator maybe) */
>  uint32_t *tmp, size;
>  size = ((csg->nrelocs + 1) * sizeof(struct radeon_bo*));
>  tmp = (uint32_t*)realloc(csg->relocs_bo, size);
> @@ -268,7 +268,7 @@ static int cs_gem_write_reloc(struct 

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 02:30:53PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2016 at 12:56:53PM +, 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 ffc974c7c300
[drm:drm_mode_object_reference] OBJ ID: 43 (1)
[drm:drm_atomic_get_crtc_state] Added [CRTC:24:crtc-0] ffc975e59400 state 
to ffc974c7c300
[drm:drm_mode_object_reference] OBJ ID: 38 (3)
[drm:drm_atomic_get_plane_state] Added [PLANE:23:plane-0] ffc974c7c100 
state to ffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 43 (2)
[drm:drm_atomic_set_mode_for_crtc] Set [NOMODE] for CRTC state ffc975e59400
[drm:drm_atomic_set_crtc_for_plane] Link plane state ffc974c7c100 to 
[NOCRTC]
[drm:drm_mode_object_unreference] OBJ ID: 38 (4)
[drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane state ffc974c7c100
[drm:drm_atomic_add_affected_connectors] Adding all current connectors for 
[CRTC:24:crtc-0] to ffc974c7c300
[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] ffc974c7c000 
state to ffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 26 (6)
[drm:drm_atomic_set_crtc_for_connector] Link connector state ffc974c7c000 
to [NOCRTC]
[drm:drm_atomic_check_only] checking ffc974c7c300
[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 ffc974c7c300
[drm:drm_atomic_commit] commiting ffc974c7c300
[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 ffc974c7c300
[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 ffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 38 (2)
[drm:drm_mode_object_unreference] OBJ ID: 38 (1)


-- 
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.

[Bug 98795] Rendering regression in radeonsi running mad max

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98795

--- Comment #1 from higuita at gmx.net ---
probably related to https://bugs.freedesktop.org/show_bug.cgi?id=98784

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161121/5905e967/attachment.html>

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:56:53PM +, 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...

-- 
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.

[Intel-gfx] [PATCH 37/37] drm/i915: Implement .get_format_info() hook for CCS

[Tvrtko Ursulin]

On 21/11/2016 13:27, Ville Syrjälä wrote:
> On Mon, Nov 21, 2016 at 08:42:13AM +, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 18/11/2016 19:53, ville.syrjala at linux.intel.com wrote:
>>> From: Ville Syrjälä 
>>>
>>> By providing our own format information for the CCS formats, we should
>>> be able to make framebuffer_check() do the right thing for the CCS
>>> surface as well.
>>>
>>> Note that we'll return the same format info for both Y and Yf tiled
>>> format as that's what happens with the non-CCS Y vs. Yf as well. If
>>> desired, we could potentially return a unique pointer for each
>>> pixel_format+tiling+ccs combination, in which case we immediately be
>>> able to tell if any of that stuff changed by just comparing the
>>> pointers. But that does sound a bit wasteful space wise.
>>>
>>> Cc: Ben Widawsky 
>>> Cc: intel-gfx at lists.freedesktop.org
>>> Signed-off-by: Ville Syrjälä 
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 37 
>>> 
>>>  include/uapi/drm/drm_fourcc.h|  3 +++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index 7b7135be3b9e..de6909770c68 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2488,6 +2488,42 @@ static unsigned int 
>>> intel_fb_modifier_to_tiling(uint64_t fb_modifier)
>>> }
>>>  }
>>>
>>> +static const struct drm_format_info ccs_formats[] = {
>>> +   { .format = DRM_FORMAT_XRGB, .depth = 24, .num_planes = 2, .cpp = { 
>>> 4, 1, }, .hsub = 16, .vsub = 8, },
>>> +   { .format = DRM_FORMAT_XBGR, .depth = 24, .num_planes = 2, .cpp = { 
>>> 4, 1, }, .hsub = 16, .vsub = 8, },
>>> +   { .format = DRM_FORMAT_ARGB, .depth = 32, .num_planes = 2, .cpp = { 
>>> 4, 1, }, .hsub = 16, .vsub = 8, },
>>> +   { .format = DRM_FORMAT_ABGR, .depth = 32, .num_planes = 2, .cpp = { 
>>> 4, 1, }, .hsub = 16, .vsub = 8, },
>>> +};
>>> +
>>> +static const struct drm_format_info *
>>> +lookup_format_info(const struct drm_format_info formats[],
>>> +  int num_formats, u32 format)
>>> +{
>>> +   int i;
>>> +
>>> +   for (i = 0; i < num_formats; i++) {
>>> +   if (formats[i].format == format)
>>> +   return [i];
>>> +   }
>>> +
>>> +   return NULL;
>>> +}
>>> +
>>> +static const struct drm_format_info *
>>> +intel_get_format_info(struct drm_device *dev,
>>> + const struct drm_mode_fb_cmd2 *cmd)
>>> +{
>>> +   switch (cmd->modifier[0]) {
>>> +   case I915_FORMAT_MOD_Y_TILED_CCS:
>>> +   case I915_FORMAT_MOD_Yf_TILED_CCS:
>>> +   return lookup_format_info(ccs_formats,
>>> + ARRAY_SIZE(ccs_formats),
>>> + cmd->pixel_format);
>>> +   default:
>>> +   return NULL;
>>> +   }
>>> +}
>>> +
>>>  static int
>>>  intel_fill_fb_info(struct drm_i915_private *dev_priv,
>>>struct drm_framebuffer *fb)
>>> @@ -15922,6 +15958,7 @@ intel_user_framebuffer_create(struct drm_device 
>>> *dev,
>>>
>>>  static const struct drm_mode_config_funcs intel_mode_funcs = {
>>> .fb_create = intel_user_framebuffer_create,
>>> +   .get_format_info = intel_get_format_info,
>>> .output_poll_changed = intel_fbdev_output_poll_changed,
>>> .atomic_check = intel_atomic_check,
>>> .atomic_commit = intel_atomic_commit,
>>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>>> index a5890bf44c0a..2926d916f199 100644
>>> --- a/include/uapi/drm/drm_fourcc.h
>>> +++ b/include/uapi/drm/drm_fourcc.h
>>> @@ -218,6 +218,9 @@ extern "C" {
>>>   */
>>>  #define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
>>>
>>> +#define I915_FORMAT_MOD_Y_TILED_CCSfourcc_mod_code(INTEL, 4)
>>> +#define I915_FORMAT_MOD_Yf_TILED_CCS   fourcc_mod_code(INTEL, 5)
>>> +
>>
>> I think when fb modifiers were started the idea was that we would later
>> partition our vendor bit space for different classes of things and have
>> helper functions to extract the tiling, etc, from them.
>>
>> For example have first 3-4 bits represent the tiling, then in this case
>> one bit for CCS, etc.
>>
>> Have you considered that when adding these ones, and concluded this
>> different scheme is better for some reason?
>
> I haven't considered anything. And obviously this patch isn't meant
> for inclusion as is. I just needed sometime to make it compile.

No idea on the status of this series. Just noticed new modifiers by 
accident and remembered the early discussions.

> Generally I don't think adding magic meaning for individual bits for
> things like this is a particularly good idea. Every time I've seen a
> scheme like that it has eventually turned ugly on account of running
> out of bits in one place or another.

I think in this case it might be much better. You just need one more 
feature which intersects with tiling and ccs to make the list not 

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 01:50:31PM +, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2016 at 12:56:53PM +, 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.

-- 
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.

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 01:24:19PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:56:53PM +, 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.

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 12:56:53PM +, 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.

-- 
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.

[PATCH] drm/atomic: cleanup debugfs entries on un-registering the driver.

[Sean Paul]

On Thu, Nov 17, 2016 at 7:26 AM, Brian Starkey  wrote:
> On Thu, Nov 17, 2016 at 11:41:29AM +, Liviu Dudau wrote:
>>
>> Cleanup the debugfs entries created by commit 6559c901cb48 when
>> the driver's minor gets un-registered. Without it, DRM drivers
>> compiled as modules cannot be rmmod-ed and modprobed again.
>>
>> Signed-off-by: Liviu Dudau 
>
>
> Works for me,
>
> Tested-by: Brian Starkey 
>

Applied to drm-misc

Thanks,

Sean

>
>> ---
>> drivers/gpu/drm/drm_atomic.c  | 7 +++
>> drivers/gpu/drm/drm_debugfs.c | 9 +
>> include/drm/drm_atomic.h  | 1 +
>> 3 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 6773b35..dddf37a 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1681,6 +1681,13 @@ int drm_atomic_debugfs_init(struct drm_minor
>> *minor)
>> ARRAY_SIZE(drm_atomic_debugfs_list),
>> minor->debugfs_root, minor);
>> }
>> +
>> +int drm_atomic_debugfs_cleanup(struct drm_minor *minor)
>> +{
>> +   return drm_debugfs_remove_files(drm_atomic_debugfs_list,
>> +
>> ARRAY_SIZE(drm_atomic_debugfs_list),
>> +   minor);
>> +}
>> #endif
>>
>> /*
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 206a4fe..2e3e46a 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -228,6 +228,7 @@ EXPORT_SYMBOL(drm_debugfs_remove_files);
>> int drm_debugfs_cleanup(struct drm_minor *minor)
>> {
>> struct drm_device *dev = minor->dev;
>> +   int ret;
>>
>> if (!minor->debugfs_root)
>> return 0;
>> @@ -235,6 +236,14 @@ int drm_debugfs_cleanup(struct drm_minor *minor)
>> if (dev->driver->debugfs_cleanup)
>> dev->driver->debugfs_cleanup(minor);
>>
>> +   if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>> +   ret = drm_atomic_debugfs_cleanup(minor);
>> +   if (ret) {
>> +   DRM_ERROR("DRM: Failed to remove atomic debugfs
>> entries\n");
>> +   return ret;
>> +   }
>> +   }
>> +
>> drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
>> minor);
>>
>> debugfs_remove(minor->debugfs_root);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 2409144..6400df0 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -374,6 +374,7 @@ void drm_state_dump(struct drm_device *dev, struct
>> drm_printer *p);
>> #ifdef CONFIG_DEBUG_FS
>> struct drm_minor;
>> int drm_atomic_debugfs_init(struct drm_minor *minor);
>> +int drm_atomic_debugfs_cleanup(struct drm_minor *minor);
>> #endif
>>
>> #define for_each_connector_in_state(__state, connector, connector_state,
>> __i) \
>> --
>> 2.10.0
>>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Manasi Navare]

On Mon, Nov 21, 2016 at 08:46:19PM +, Chris Wilson wrote:
> On Mon, Nov 21, 2016 at 11:00:52AM -0800, Manasi Navare wrote:
> > On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > > > - Another fallout is that legacy clients will no longer see the
> > > > > >   link-status property. And they won't be able to set it through the
> > > > > >   SETCRTC ioctl, which would kinda defaut the point. I think the 
> > > > > > best
> > > > > >   solution would be to check for link_status == BAD in
> > > > > >   drm_atomic_helper_set_config, and reset it to good automatically 
> > > > > > for
> > > > > >   legacy clients.
> > > > > 
> > > > > Then how do they know that the kernel demands the modeset? Both a 
> > > > > legacy
> > > > > and atomic property?
> > > > 
> > > > I guess we could avoid the filtering of the property for legacy clients.
> > > > Definitely not 2 properties, that's silly. Or we teach userspace to go
> > > > look for atomic properties.
> > > 
> > > Well, now that I flushed the gunk out of my brain with some work-out it's
> > > a lot easier: ATOMIC on properties is only to hide them from legacy
> > > userspace, it doesn't control how it's implement. Which means we can
> > > implement it as described above, and non-atomic userspace can still read
> > > it. Setting would also work, but since we want to do that as part of
> > > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a
> > > modeset will happen or not, automagic in there seems reasonable.
> > 
> > Thanks Daniel for providing the solution alternatives here.
> > So after we make it atomic, we would solve the problem of updating the 
> > connector_changed
> > in atomic_helper_check_modeset function. So in this, who resets the 
> > property to GOOD?
> > Would this happen in drm_atomic_helper_set_config in both atomic and non 
> > atomic cases?
> > 
> > And in case of non atomic userspace, will it still be able to read 
> > link-status as BAD in userspace
> > to decide whether it needs to call setcrtc?
> > 
> > Chris, will any implementation in your patch for link _status change if 
> > this is made atomic?
> 
> So long at the property remains visible via the GETCONNECTOR ioctl, no.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

If it is made an atomic property, will it be visible to userspace through
GETCONNECTOR?

Manasi

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 12:25:56PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 11:32:12AM +, Liviu Dudau wrote:
> > On Mon, Nov 21, 2016 at 11:20:30AM +, 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 0001
> [  239.264672] hdlcd_crtc_disable: active 0
> [  239.285180] hdlcd_crtc_enable: active 1 cmd 0001
> [  278.712792] hdlcd_crtc_disable: active 0
> [  278.730361] hdlcd_crtc_enable: active 1 cmd 0001
> [  281.633841] hdlcd_crtc_disable: active 0
> [  281.668578] hdlcd_crtc_enable: active 1 cmd 0001
> 
> 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_cntrate   
> accuracy
>   phase
> 
>  pxlclk   66   14850  
> 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 

[PATCH] drm/fences: add DOC: for explicit fencing

[Gustavo Padovan]

From: Gustavo Padovan 

Document IN_FENCE_FD and OUT_FENCE_PTR properties.

Signed-off-by: Gustavo Padovan 
---
 Documentation/gpu/drm-kms.rst |  6 ++
 drivers/gpu/drm/drm_atomic.c  | 31 +++
 2 files changed, 37 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 568f3c2..cdc9539 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -287,6 +287,12 @@ Tile Group Property
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
:doc: Tile group

+Explicit Fencing Properties
+---
+
+.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
+   :doc: explicit fencing properties
+
 Existing KMS Properties
 ---

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b476ec5..7f33031 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1809,6 +1809,37 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+/**
+ * DOC: explicit fencing properties
+ *
+ * Explicit fencing allows userspace to control the buffer synchronization
+ * between devices. A Fence or a group of fences are trasnfered to/from
+ * userspace using Sync File fds and there are two DRM properties for that.
+ * IN_FENCE_FD on each DRM Plane to send fences to the kernel and
+ * OUT_FENCE_PTR on each DRM CRTC to receive fences from the kernel.
+ *
+ * "IN_FENCE_FD”:
+ * Use this property to pass a fence that DRM should wait on before
+ * proceeding with the Atomic Commit request and show the framebuffer for
+ * the plane on the screen. The fence can be either a normal fence or a
+ * merged one, the sync_file framework will handle both case and use a
+ * fence_array if a merged fence is received. Passing -1 here means no
+ * fences to wait on.
+ *
+ * "OUT_FENCE_PTR”:
+ * Use this property to pass a file descriptor pointer to DRM. Once the
+ * Atomic Commit request call returns OUT_FENCE_PTR will be filled with
+ * the file descriptor number of a Sync File. This Sync File contains the
+ * CRTC fence that will be signaled when all framebuffers present on the
+ * Atomic Commit * request for that given CRTC are scanned out on the
+ * screen.
+ *
+ * The Atomic Commit request fails if a invalid pointer is passed. If the
+ * Atomic Commit request fails for any other reason the out fence fd
+ * returned will be -1. On a Atomic Commit with the
+ * DRM_MODE_ATOMIC_TEST_ONLY flag the out fence will also be set to -1.
+ */
+
 static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc)
 {
struct dma_fence *fence;
-- 
2.5.5

[PATCH] drm/arm: hdlcd: fix plane base address calculation

[Russell King]

The plane base address needs to be calculated using the source
coordinates to position the source correctly - it's possible to have
a larger source buffer than the CRTC size, and have several CRTCs
reading from different parts of the buffer.

In such a case, the pitch may be larger, and we will use the source
position to select an area of the buffer to scan out.

In order for this to work correctly, we need to also fix the atomic
check to do a fuller validation of the new state.

Signed-off-by: Russell King 
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 41 
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 48019ae22ddb..c239616f5334 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -10,6 +10,7 @@
  */

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -206,13 +207,30 @@ static const struct drm_crtc_helper_funcs 
hdlcd_crtc_helper_funcs = {
 static int hdlcd_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *state)
 {
-   u32 src_w, src_h;
+   struct drm_crtc_state *crtc_state;
+   struct drm_crtc *crtc;
+   struct drm_rect clip = { 0 };
+   int ret;

-   src_w = state->src_w >> 16;
-   src_h = state->src_h >> 16;
+   crtc = state->crtc;
+   if (!crtc)
+   return 0;

-   /* we can't do any scaling of the plane source */
-   if ((src_w != state->crtc_w) || (src_h != state->crtc_h))
+   crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
+   if (!crtc_state->enable)
+   return -EINVAL;
+
+   clip.x2 = crtc_state->adjusted_mode.hdisplay;
+   clip.y2 = crtc_state->adjusted_mode.vdisplay;
+
+   ret = drm_plane_helper_check_state(state, ,
+  DRM_PLANE_HELPER_NO_SCALING,
+  DRM_PLANE_HELPER_NO_SCALING,
+  false, true);
+   if (ret)
+   return ret;
+
+   if (!state->visible)
return -EINVAL;

return 0;
@@ -224,21 +242,20 @@ static void hdlcd_plane_atomic_update(struct drm_plane 
*plane,
struct hdlcd_drm_private *hdlcd;
struct drm_gem_cma_object *gem;
unsigned int depth, bpp;
-   u32 src_w, src_h, dest_w, dest_h;
+   u32 src_x, src_y, dest_h;
dma_addr_t scanout_start;

if (!plane->state->fb)
return;

drm_fb_get_bpp_depth(plane->state->fb->pixel_format, , );
-   src_w = plane->state->src_w >> 16;
-   src_h = plane->state->src_h >> 16;
-   dest_w = plane->state->crtc_w;
-   dest_h = plane->state->crtc_h;
gem = drm_fb_cma_get_gem_obj(plane->state->fb, 0);
+   src_x = plane->state->src_x >> 16;
+   src_y = plane->state->src_y >> 16;
+   dest_h = plane->state->crtc_h;
scanout_start = gem->paddr + plane->state->fb->offsets[0] +
-   plane->state->crtc_y * plane->state->fb->pitches[0] +
-   plane->state->crtc_x * bpp / 8;
+   src_y * plane->state->fb->pitches[0] +
+   src_x * bpp / 8;

hdlcd = plane->dev->dev_private;
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, 
plane->state->fb->pitches[0]);
-- 
2.7.4

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 11:32:12AM +, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 11:20:30AM +, 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 0001
[  239.264672] hdlcd_crtc_disable: active 0
[  239.285180] hdlcd_crtc_enable: active 1 cmd 0001
[  278.712792] hdlcd_crtc_disable: active 0
[  278.730361] hdlcd_crtc_enable: active 1 cmd 0001
[  281.633841] hdlcd_crtc_disable: active 0
[  281.668578] hdlcd_crtc_enable: active 1 cmd 0001

So, when ->disable is called, active is always zero.  This
leads to...

$ head -n3 /sys/kernel/debug/clk/clk_summary
   clock enable_cnt  prepare_cntrate   accuracy
  phase

 pxlclk   66   14850  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?

-- 
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.

[PATCHv3 3/4] drm/tilcdc: Use unload to handle initialization failures

[Jyri Sarha]

On 11/18/16 18:57, Bartosz Golaszewski wrote:
> 2016-11-02 16:57 GMT+01:00 Jyri Sarha :
>> Use unload to handle initialization failures instead of complex goto
>> label mess. To do this the initialization sequence needed slight
>> reordering and some unload functions needed to become conditional.
>>
>> Signed-off-by: Jyri Sarha 
>> ---
> 
> I'm not sure yet of the exact error path, but with this patch
> tilcdc_crtc_destroy() fails with a NULL-pointer dereference at
> dmam_free_coherent() due to crtc->dev being NULL if there are no
> panels registered.
> 

Argh, should have read the dmam_alloc_coherent() function documentation.
I just wondered what the extra m in function prefix was for and did not
realize that it was a devres version of the function (I would have
expected such a function to be called devm_dma_alloc_coherent()).

Anyway, I'll drop the "drm/tilcdc: Free palette dma memory in
tilcdc_crtc_destroy()" patch.

Thanks,
Jyri

[PATCH v2 00/37] drm: Deduplicate fb format information (v2)

[Christian König]

Patches #2 and #3 are Reviewed-by: Christian König 
.

The rest is Acked-by: Christian König .

Regards,
Christian.

Am 18.11.2016 um 20:52 schrieb ville.syrjala at linux.intel.com:
> From: Ville Syrjälä 
>
> Second installment of my effort to remove the duplicated
> depth/bpp/pixel_format from drm_framebuffer and just use
> struct drm_format_info instead.
>
> I tried to address all of the review feedback, and collect
> up all the r-bs I already got. Thanks for the review, guys.
>
> Changes since the last version are roughly:
> * drm_framebuffer_init() now fails if the fb isn't properly prepared
> * Applied mode cocciry all over to use fb->format more extensively
> * Dropped a few i915 specific patches that were taken care of the
>previous item
> * Took up Laurent's idea that we can just compare the fb->format
>pointers instead of comparing the fb->format->format values
> * Added a new .get_format_info() hooks for drivers to provide custom
>format information + an quick example patch how we'd hook it up
>for i915 render compression support
>
> Link to the previous version:
> https://lists.freedesktop.org/archives/dri-devel/2016-November/124135.html
>
> Entire series is available here:
> git://github.com/vsyrjala/linux.git fb_format_dedup_2
>
> Cc: Alex Deucher 
> Cc: Alexey Brodkin 
> Cc: Ben Skeggs 
> Cc: Ben Widawsky 
> Cc: Brian Starkey 
> Cc: "Christian König" 
> Cc: Dave Airlie 
> Cc: Gerd Hoffmann 
> Cc: intel-gfx at lists.freedesktop.org
> Cc: Laurent Pinchart 
> Cc: linux-graphics-maintainer at vmware.com
> Cc: Liviu Dudau 
> Cc: Mali DP Maintainers 
> Cc: Patrik Jakobsson 
> Cc: Paulo Zanoni 
> Cc: Sinclair Yeh 
> Cc: Thomas Hellstrom 
>
> Ville Syrjälä (37):
>drm/i915: Add local 'fb' variables
>drm/radeon: Add local 'fb' variables
>drm/radeon: Use DIV_ROUND_UP()
>drm/mgag200: Add local 'fb' variable
>drm/ast: Add local 'fb' variables
>drm/gma500: Add some local 'fb' variables
>drm/cirrus: Add some local 'fb' variables
>drm/arcpgu: Add local 'fb' variables
>drm/arm: Add local 'fb' variables
>drm/nouveau: Fix crtc->primary->fb vs. drm_fb fail
>drm/nouveau: Add local 'fb' variables
>drm/vmwgfx: Populate fb->dev before drm_framebuffer_init()
>drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()
>drm/vmwgfx: Populate fb->pixel_format
>drm/qxl: Call drm_helper_mode_fill_fb_struct() before
>  drm_framebuffer_init()
>drm/virtio: Call drm_helper_mode_fill_fb_struct() before
>  drm_framebuffer_init()
>drm/i915: Set fb->dev early on for inherited fbs
>drm: Populate fb->dev from drm_helper_mode_fill_fb_struct()
>drm: Store a pointer to drm_format_info under drm_framebuffer
>drm/vmwgfx: Populate fb->format correctly
>drm/i915: Populate fb->format early for inherited fbs
>drm: Reject fbs w/o format info in drm_framebuffer_init()
>drm: Replace drm_format_num_planes() with fb->format->num_planes
>drm/i915: Eliminate the ugly 'fb?:' constructs from the ilk/skl wm
>  code
>drm: Replace drm_format_plane_cpp() with fb->format->cpp[]
>drm/fb_cma_helper: Replace drm_format_info() with fb->format
>drm/nouveau: Use fb->format rather than drm_format_info()
>drm/i915: Store a pointer to the pixel format info for fbc
>drm: Add drm_framebuffer_plane_{width,height}()
>drm/i915: Use drm_framebuffer_plane_{width,height}() where possible
>drm: Nuke fb->depth
>drm: Nuke fb->bits_per_pixel
>drm: Nuke fb->pixel_format
>drm: Replace 'format->format' comparisons to just 'format' comparisons
>drm: Eliminate the useless "non-RGB fb" debug message
>drm: Add mode_config .get_format_info() hook
>drm/i915: Implement .get_format_info() hook for CCS
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |   2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  |   4 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  |   6 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v11_0.c  |   6 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v6_0.c   |   6 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   |   6 +-
>   drivers/gpu/drm/arc/arcpgu_crtc.c   |   3 +-
>   drivers/gpu/drm/arm/hdlcd_crtc.c|  18 ++--
>   drivers/gpu/drm/arm/malidp_planes.c |  10 +--
>   drivers/gpu/drm/armada/armada_crtc.c|   6 +-
>   drivers/gpu/drm/armada/armada_fb.c  |   2 +-
>   drivers/gpu/drm/armada/armada_fbdev.c   |   5 +-
>   drivers/gpu/drm/armada/armada_overlay.c |   6 +-
>   drivers/gpu/drm/ast/ast_fb.c|   4 +-
>   drivers/gpu/drm/ast/ast_main.c  |   2 +-
>   drivers/gpu/drm/ast/ast_mode.c  |  16 ++--
>   drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c |   2 +-
>   drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  22 ++---
>   drivers/gpu/drm/bochs/bochs_fbdev.c |   2 +-
>   drivers/gpu/drm/bochs/bochs_mm.c|   2 +-
>   

[PATCH v2 09/13] arm64: dts: r8a7795: salvator-x: Add panel backlight support

[Laurent Pinchart]

Hi Geert,

On Monday 21 Nov 2016 10:23:46 Geert Uytterhoeven wrote:
> On Mon, Nov 21, 2016 at 10:19 AM, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 09:36:22 Geert Uytterhoeven wrote:
> >> On Sat, Nov 19, 2016 at 4:28 AM, Laurent Pinchart wrote:
> >>> The panel backlight is controlled through a GPIO and a PWM channel.
> >>> 
> >>> --- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
> >>> +++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
> >>> @@ -178,6 +178,16 @@
> >>> };
> >>> };
> >>> };
> >>> +
> >>> +   backlight: backlight {
> >>> +   compatible = "pwm-backlight";
> >>> +   pwms = < 0 5>;
> >>> +
> >>> +   brightness-levels = <256 128 64 16 8 4 0>;
> >> 
> >> Would it make sense to define more and/or linear levels?
> > 
> > Possibly, this is pretty arbitrary. Linear levels might not be the best
> > option given that the human eye doesn't have a linear response to light
> > power, but we
>
> It not only depends on the human eye, but also on the backlight hardware
> (is the conversion from voltage (L_VBRT) to light linear?).

So we need to specify transfer functions in DT ;-)

> > could certainly have more levels. In that case I'd prefer modifying the
> > pwm- backlight DT bindings though, and specifying the PWM resolution
> > instead of discrete levels.
> > 
> > Note that the LVDS panel backlight PWM control signal is multiplexed with
> > the external memory A21 signal on the Salvator-X board, with SW5
> > selecting which how to route the signal. When using backlight control we
> > can't access the whole NOR flash anymore, so I'm not sure this patch
> > should be merged.
>
> That NOR flash is also optional, right?
> My Ex Memory Connector is not populated.

That's correct. The Salvator-X DT file in mainline is just an example anyway, 
and we should pick the most useful peripherals for that purpose.

-- 
Regards,

Laurent Pinchart

[PATCH 09/37] drm/arm: Add local 'fb' variables

[Liviu Dudau]

On Fri, Nov 18, 2016 at 09:52:45PM +0200, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrjälä 
> 
> Add a local 'fb' variable to a few places to get rid of the
> 'crtc->primary->fb' stuff. Looks neater and helps me with my ppor
> coccinelle skills later.
> 
> In some places the local variable was already there, just not used
> consistently.
> 
> Cc: Liviu Dudau 

Acked-by: Liviu Dudau 

Are you going to take the series through drm-misc or you want each 
sub-maintainer
to cherry pick the patches?

Best regards,
Liviu

> Cc: Brian Starkey 
> Cc: Mali DP Maintainers 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c| 18 ++
>  drivers/gpu/drm/arm/malidp_planes.c |  6 +++---
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c 
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index bbaa55add2d2..8a0fee03aa39 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -60,11 +60,12 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc)
>  {
>   unsigned int btpp;
>   struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> + const struct drm_framebuffer *fb = crtc->primary->state->fb;
>   uint32_t pixel_format;
>   struct simplefb_format *format = NULL;
>   int i;
>  
> - pixel_format = crtc->primary->state->fb->pixel_format;
> + pixel_format = fb->pixel_format;
>  
>   for (i = 0; i < ARRAY_SIZE(supported_formats); i++) {
>   if (supported_formats[i].fourcc == pixel_format)
> @@ -221,27 +222,28 @@ static int hdlcd_plane_atomic_check(struct drm_plane 
> *plane,
>  static void hdlcd_plane_atomic_update(struct drm_plane *plane,
> struct drm_plane_state *state)
>  {
> + struct drm_framebuffer *fb = plane->state->fb;
>   struct hdlcd_drm_private *hdlcd;
>   struct drm_gem_cma_object *gem;
>   u32 src_w, src_h, dest_w, dest_h;
>   dma_addr_t scanout_start;
>  
> - if (!plane->state->fb)
> + if (!fb)
>   return;
>  
>   src_w = plane->state->src_w >> 16;
>   src_h = plane->state->src_h >> 16;
>   dest_w = plane->state->crtc_w;
>   dest_h = plane->state->crtc_h;
> - gem = drm_fb_cma_get_gem_obj(plane->state->fb, 0);
> - scanout_start = gem->paddr + plane->state->fb->offsets[0] +
> - plane->state->crtc_y * plane->state->fb->pitches[0] +
> + gem = drm_fb_cma_get_gem_obj(fb, 0);
> + scanout_start = gem->paddr + fb->offsets[0] +
> + plane->state->crtc_y * fb->pitches[0] +
>   plane->state->crtc_x *
> - drm_format_plane_cpp(plane->state->fb->pixel_format, 0);
> + drm_format_plane_cpp(fb->pixel_format, 0);
>  
>   hdlcd = plane->dev->dev_private;
> - hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, 
> plane->state->fb->pitches[0]);
> - hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, 
> plane->state->fb->pitches[0]);
> + hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]);
> + hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
>   hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
>   hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
>  }
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c 
> b/drivers/gpu/drm/arm/malidp_planes.c
> index 63eec8f37cfc..ee7f7663a307 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -137,8 +137,8 @@ static int malidp_de_plane_check(struct drm_plane *plane,
>  
>   /* packed RGB888 / BGR888 can't be rotated or flipped */
>   if (state->rotation != DRM_ROTATE_0 &&
> - (state->fb->pixel_format == DRM_FORMAT_RGB888 ||
> -  state->fb->pixel_format == DRM_FORMAT_BGR888))
> + (fb->pixel_format == DRM_FORMAT_RGB888 ||
> +  fb->pixel_format == DRM_FORMAT_BGR888))
>   return -EINVAL;
>  
>   ms->rotmem_size = 0;
> @@ -147,7 +147,7 @@ static int malidp_de_plane_check(struct drm_plane *plane,
>  
>   val = mp->hwdev->rotmem_required(mp->hwdev, state->crtc_h,
>state->crtc_w,
> -  state->fb->pixel_format);
> +  fb->pixel_format);
>   if (val < 0)
>   return val;
>  
> -- 
> 2.7.4
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[PATCHv3 3/4] drm/tilcdc: Use unload to handle initialization failures

[Bartosz Golaszewski]

2016-11-21 11:24 GMT+01:00 Jyri Sarha :
> On 11/18/16 18:57, Bartosz Golaszewski wrote:
>> 2016-11-02 16:57 GMT+01:00 Jyri Sarha :
>>> Use unload to handle initialization failures instead of complex goto
>>> label mess. To do this the initialization sequence needed slight
>>> reordering and some unload functions needed to become conditional.
>>>
>>> Signed-off-by: Jyri Sarha 
>>> ---
>>
>> I'm not sure yet of the exact error path, but with this patch
>> tilcdc_crtc_destroy() fails with a NULL-pointer dereference at
>> dmam_free_coherent() due to crtc->dev being NULL if there are no
>> panels registered.
>>
>
> Argh, should have read the dmam_alloc_coherent() function documentation.
> I just wondered what the extra m in function prefix was for and did not
> realize that it was a devres version of the function (I would have
> expected such a function to be called devm_dma_alloc_coherent()).
>

I don't get it either - the original commit introducing devres
(9ac7849e35f7: "devres: device resource management") already had
different prefixes for different managed interfaces.

Maybe we should propose renaming them unless there's a good reason to
keep the dmam prefix?

Thanks,
Bartosz

[PATCH v5 1/2] dt-bindings: display: Add Sharp LQ150X1LG11 panel binding

[Rob Herring]

On Mon, Nov 21, 2016 at 04:00:48PM +0100, Peter Rosin wrote:
> The Sharp 15" LQ150X1LG11 panel is an XGA TFT LCD panel.
> 
> Signed-off-by: Peter Rosin 
> ---
>  .../bindings/display/panel/sharp,lq150x1lg11.txt   | 36 
> ++
>  1 file changed, 36 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/sharp,lq150x1lg11.txt

Acked-by: Rob Herring 

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Mon, Nov 21, 2016 at 11:20:30AM +, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 11:06:04AM +, Liviu Dudau wrote:
> > On Fri, Nov 18, 2016 at 11:37:33PM +, 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 0xf500 - 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 0xf500, 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.

[BUG] hdlcd gets confused about base address

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 11:06:04AM +, Liviu Dudau wrote:
> On Fri, Nov 18, 2016 at 11:37:33PM +, 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 0xf500 - 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 0xf500, 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);
> 

[PATCH v2 09/13] arm64: dts: r8a7795: salvator-x: Add panel backlight support

[Laurent Pinchart]

Hi Geert,

On Monday 21 Nov 2016 09:36:22 Geert Uytterhoeven wrote:
> On Sat, Nov 19, 2016 at 4:28 AM, Laurent Pinchart wrote:
> > The panel backlight is controlled through a GPIO and a PWM channel.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Reviewed-by: Geert Uytterhoeven 
> 
> > --- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
> > @@ -178,6 +178,16 @@
> > };
> > };
> > };
> > +
> > +   backlight: backlight {
> > +   compatible = "pwm-backlight";
> > +   pwms = < 0 5>;
> > +
> > +   brightness-levels = <256 128 64 16 8 4 0>;
> 
> Would it make sense to define more and/or linear levels?

Possibly, this is pretty arbitrary. Linear levels might not be the best option 
given that the human eye doesn't have a linear response to light power, but we 
could certainly have more levels. In that case I'd prefer modifying the pwm-
backlight DT bindings though, and specifying the PWM resolution instead of 
discrete levels.

Note that the LVDS panel backlight PWM control signal is multiplexed with the 
external memory A21 signal on the Salvator-X board, with SW5 selecting which 
how to route the signal. When using backlight control we can't access the 
whole NOR flash anymore, so I'm not sure this patch should be merged.

> > +   default-brightness-level = <6>;

-- 
Regards,

Laurent Pinchart

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Daniel Vetter]

On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst 
> > > > > > > > wrote:
> > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > >>> if connector link status property has changed. This will tell 
> > > > > > > >>> the
> > > > > > > >>> driver to do a complete modeset due to change in connector 
> > > > > > > >>> property.
> > > > > > > >>>
> > > > > > > >>> Acked-by: Harry Wentland 
> > > > > > > >>> Acked-by: Tony Cheng 
> > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > >>> Cc: Jani Nikula 
> > > > > > > >>> Cc: Daniel Vetter 
> > > > > > > >>> Cc: Ville Syrjala 
> > > > > > > >>> Signed-off-by: Manasi Navare 
> > > > > > > >>> ---
> > > > > > > >>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > >>>  1 file changed, 7 insertions(+)
> > > > > > > >>>
> > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> @@ -519,6 +519,13 @@ static int 
> > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > > > >>>  connector_state);
> > > > > > > >>>   if (ret)
> > > > > > > >>>   return ret;
> > > > > > > >>> +
> > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > >>> + crtc_state = 
> > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > >>> + 
> > > > > > > >>> connector->state->crtc);
> > > > > > > >>> + if (connector->link_status == 
> > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > >>> + crtc_state->connectors_changed 
> > > > > > > >>> = true;
> > > > > > > >>> + }
> > > > > > > >>>   }
> > > > > > > >>>  
> > > > > > > >>>   /*
> > > > > > > >> This will cause ordinary atomic commits that happen to change 
> > > > > > > >> connector flags to potentially fail with -EINVAL if 
> > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > >> For this reason I'm not sure this flag should be set 
> > > > > > > >> automatically by the kernel. Could we add add a retrain link 
> > > > > > > >> property instead, that
> > > > > > > >> always return 0 when queried, but writing a 1 causing 
> > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > Or just check for allow_modeset before setting 
> > > > > > > > connectors_changed=true here?
> > > > > > > 
> > > > > > > I don't think modesets should be done automatically like that, 
> > > > > > > even if ALLOW_MODESET is set a modeset may not be expected by 
> > > > > > > userspace.
> > > > > > 
> > > > > > Presumably userspace would want a picture on the screen using any 
> > > > > > means
> > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it 
> > > > > > should
> > > > > > probably say as much?
> > > > > 
> > > > > Yeah, agreed. Also, if the link is bad then we pretty much have to do 
> > > > > a
> > > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > > screen.
> > > > > 
> > > > > What we could try is to gate this of whether userspace touches the 
> > > > > mode
> > > > > property on the corresponding CRTC. I.e. if that's touched (even if 
> > > > > it's
> > > > > the same mode), and a link is bad in one of the connectors in the 
> > > > > state
> > > > > then we do a full modeset to recover.
> > > > > 
> > > > > Another option would be to make the link status writeable. Trying to
> > > > > change it from bad->good would force the modeset. That would be 100% 
> > > > > clear
> > > > > to userspace, not special hacks needed with checking for 
> > > > > allow_modeset,
> > > > > no magic property that auto-changes its value. And 100% backwards 
> > > > > compat
> > > > > because existing userspace should never touch properties it doesn't
> > > > > understand (except when restoring a mode, and then it should allow a 
> > > > > full
> > > > > modeset). And if someone does try a good->bad transition, we just 
> > > > > silently
> > > > > keep it at good.
> > > > 

[PATCH v6 2/5] drm: sun8i: add HDMI video support to A83T and H3

[Rob Herring]

On Sun, Nov 20, 2016 at 10:56:23AM +0100, Jean-Francois Moine wrote:
> This patch adds a HDMI video driver to the Allwinner's SoCs A83T and H3.
> 
> Signed-off-by: Jean-Francois Moine 
> ---
>  .../devicetree/bindings/display/sunxi/hdmi.txt |  53 ++
>  drivers/gpu/drm/sun8i/Kconfig  |   7 +
>  drivers/gpu/drm/sun8i/Makefile |   2 +
>  drivers/gpu/drm/sun8i/de2_hdmi.c   | 394 ++
>  drivers/gpu/drm/sun8i/de2_hdmi.h   |  51 ++
>  drivers/gpu/drm/sun8i/de2_hdmi_io.c| 839 
> +
>  6 files changed, 1346 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/sunxi/hdmi.txt
>  create mode 100644 drivers/gpu/drm/sun8i/de2_hdmi.c
>  create mode 100644 drivers/gpu/drm/sun8i/de2_hdmi.h
>  create mode 100644 drivers/gpu/drm/sun8i/de2_hdmi_io.c
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/hdmi.txt 
> b/Documentation/devicetree/bindings/display/sunxi/hdmi.txt
> new file mode 100644
> index 000..85709ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/hdmi.txt
> @@ -0,0 +1,53 @@
> +Allwinner HDMI Transmitter
> +==
> +
> +The Allwinner HDMI transmitters are included in the SoCs.
> +They support audio and video.
> +
> +Required properties:
> + - #address-cells : should be <1>
> + - #size-cells : should be <0>
> + - compatible : should be one of
> + "allwinner,sun8i-a83t-hdmi"
> + "allwinner,sun8i-h3-hdmi"
> + - clocks : phandles to the HDMI clocks as described in
> + Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - clock-names : must be
> + "gate" : bus gate
> + "clock" : streaming clock
> + "ddc-clock" : DDC clock
> + - resets : One or two phandles to the HDMI resets
> + - reset-names : when 2 phandles, must be
> + "hdmi0" and "hdmi1"
> +
> +Required nodes:
> + - port: Audio and video input port nodes with endpoint definitions
> + as defined in Documentation/devicetree/bindings/graph.txt.
> + port at 0 is video and port at 1 is audio.

This should probably also have an output port to the hdmi-connector 
binding. It is not needed so much if this block handles DDC and HPD 
itself, but if those are a separate I2C controller and GPIO, 
respectively, then you need it for sure. There's also power on the 
connector or other connectors like muxed on Type-C. 


> +
> +Example:
> +
> + hdmi: hdmi at 01ee {
> + compatible = "allwinner,sun8i-a83t-hdmi";
> + reg = <0x01ee 0x2>;
> + clocks = < CLK_BUS_HDMI>, < CLK_HDMI>,
> +  < CLK_HDMI_DDC>;
> + clock-names = "gate", "clock", "ddc-clock";
> + resets = < RST_HDMI0>, < RST_HDMI1>;
> + reset-names = "hdmi0", "hdmi1";
> + ...

Please show all properties in example.

> + #address-cells = <1>;
> + #size-cells = <0>;
> + port at 0 { /* video */
> + reg = <0>;
> + hdmi_lcd1: endpoint {
> + remote-endpoint = <_hdmi>;
> + };
> + };
> + port at 1 { /* audio */
> + reg = <1>;
> + hdmi_i2s2: endpoint {
> + remote-endpoint = <_hdmi>;
> + };
> + };
> + };

[BUG] hdlcd gets confused about base address

[Liviu Dudau]

On Fri, Nov 18, 2016 at 11:37:33PM +, 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 
> ---
> 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 0xf500 - 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.

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Manasi Navare]

On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst 
> > > > > > > > wrote:
> > > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst 
> > > > > > > > > > wrote:
> > > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > > >>> if connector link status property has changed. This will 
> > > > > > > > > >>> tell the
> > > > > > > > > >>> driver to do a complete modeset due to change in 
> > > > > > > > > >>> connector property.
> > > > > > > > > >>>
> > > > > > > > > >>> Acked-by: Harry Wentland 
> > > > > > > > > >>> Acked-by: Tony Cheng 
> > > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > > >>> Cc: Jani Nikula 
> > > > > > > > > >>> Cc: Daniel Vetter 
> > > > > > > > > >>> Cc: Ville Syrjala 
> > > > > > > > > >>> Signed-off-by: Manasi Navare  > > > > > > > > >>> intel.com>
> > > > > > > > > >>> ---
> > > > > > > > > >>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > > >>>  1 file changed, 7 insertions(+)
> > > > > > > > > >>>
> > > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> @@ -519,6 +519,13 @@ static int 
> > > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state 
> > > > > > > > > >>> *state,
> > > > > > > > > >>>  connector_state);
> > > > > > > > > >>>   if (ret)
> > > > > > > > > >>>   return ret;
> > > > > > > > > >>> +
> > > > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > > > >>> + crtc_state = 
> > > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > > >>> + 
> > > > > > > > > >>> connector->state->crtc);
> > > > > > > > > >>> + if (connector->link_status == 
> > > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > > >>> + crtc_state->connectors_changed 
> > > > > > > > > >>> = true;
> > > > > > > > > >>> + }
> > > > > > > > > >>>   }
> > > > > > > > > >>>  
> > > > > > > > > >>>   /*
> > > > > > > > > >> This will cause ordinary atomic commits that happen to 
> > > > > > > > > >> change connector flags to potentially fail with -EINVAL if 
> > > > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > > > >> For this reason I'm not sure this flag should be set 
> > > > > > > > > >> automatically by the kernel. Could we add add a retrain 
> > > > > > > > > >> link property instead, that
> > > > > > > > > >> always return 0 when queried, but writing a 1 causing 
> > > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > > Or just check for allow_modeset before setting 
> > > > > > > > > > connectors_changed=true here?
> > > > > > > > > 
> > > > > > > > > I don't think modesets should be done automatically like 
> > > > > > > > > that, even if ALLOW_MODESET is set a modeset may not be 
> > > > > > > > > expected by userspace.
> > > > > > > > 
> > > > > > > > Presumably userspace would want a picture on the screen using 
> > > > > > > > any means
> > > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset 
> > > > > > > > it should
> > > > > > > > probably say as much?
> > > > > > > 
> > > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have 
> > > > > > > to do a
> > > > > > > modeset to recover it, otherwise you'll be forever stuck with a 
> > > > > > > bad
> > > > > > > screen.
> > > > > > > 
> > > > > > > What we could try is to gate this of whether userspace touches 
> > > > > > > the mode
> > > > > > > property on the corresponding CRTC. I.e. if that's touched (even 
> > > > > > > if it's
> > > > > > > the same mode), and a link is bad in one of the connectors in the 
> > > > > > > state
> > > > > > > then we do a full modeset to recover.
> > > > > > > 
> > > > > > > Another option would be to make the link status writeable. Trying 
> > > > > > > to
> > > > > > > change it from bad->good would force 

[PATCH v6 1/5] drm: sun8i: Add a basic DRM driver for Allwinner DE2

[Rob Herring]

On Sun, Nov 20, 2016 at 10:53:25AM +0100, Jean-Francois Moine wrote:
> Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> engine, DE2.
> This patch adds a DRM video driver for this device.
> 
> Signed-off-by: Jean-Francois Moine 
> ---
>  .../bindings/display/sunxi/sun8i-de2.txt   |  83 +++

It's preferred to split bindings to a separate patch.

>  drivers/gpu/drm/Kconfig|   2 +
>  drivers/gpu/drm/Makefile   |   1 +
>  drivers/gpu/drm/sun8i/Kconfig  |  19 +
>  drivers/gpu/drm/sun8i/Makefile |   7 +
>  drivers/gpu/drm/sun8i/de2_crtc.c   | 440 +
>  drivers/gpu/drm/sun8i/de2_crtc.h   |  50 ++
>  drivers/gpu/drm/sun8i/de2_drm.h|  48 ++
>  drivers/gpu/drm/sun8i/de2_drv.c| 379 +++
>  drivers/gpu/drm/sun8i/de2_plane.c  | 712 
> +
>  10 files changed, 1741 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
>  create mode 100644 drivers/gpu/drm/sun8i/Kconfig
>  create mode 100644 drivers/gpu/drm/sun8i/Makefile
>  create mode 100644 drivers/gpu/drm/sun8i/de2_crtc.c
>  create mode 100644 drivers/gpu/drm/sun8i/de2_crtc.h
>  create mode 100644 drivers/gpu/drm/sun8i/de2_drm.h
>  create mode 100644 drivers/gpu/drm/sun8i/de2_drv.c
>  create mode 100644 drivers/gpu/drm/sun8i/de2_plane.c
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt 
> b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> new file mode 100644
> index 000..b9edd4b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> @@ -0,0 +1,83 @@
> +Allwinner sun8i Display Engine 2 subsystem
> +==
> +
> +The Allwinner DE2 subsystem contains a display controller (DE2),
> +one or two LCD controllers (TCON) and their external interfaces.
> +
> +Display controller
> +==
> +
> +Required properties:
> +
> +- compatible: value should be one of the following
> + "allwinner,sun8i-a83t-display-engine"
> + "allwinner,sun8i-h3-display-engine"
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> + clock-names property.
> +
> +- clock-names: must contain
> + "gate": DE bus gate
> + "clock": DE clock
> +
> +- resets: phandle to the reset of the device
> +
> +- ports: phandle's to the LCD ports

This should use OF graph to describe the connection from the DE to the 
LCD controllers like the sun4i binding does.

No registers for the DE?

> +
> +LCD controller
> +==
> +
> +Required properties:
> +
> +- compatible: should be
> + "allwinner,sun8i-a83t-tcon"
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> + clock-names property.
> +
> +- clock-names: must contain
> + "gate": TCON bus gate
> + "clock": TCON pixel clock
> +
> +- resets: phandle to the reset of the device
> +
> +- port: port node with endpoint definitions as defined in
> + Documentation/devicetree/bindings/media/video-interfaces.txt

Need to specify how many ports and endpoints.

> +
> +Example:
> +
> + de: de-controller at 0100 {
> + compatible = "allwinner,sun8i-h3-display-engine";
> + ...

What are you not showing?

> + clocks = <& CLK_BUS_DE>, < CLK_DE>;
> + clock-names = "gate", "clock";
> + resets = < RST_BUS_DE>;
> + ports = <_p>;
> + };
> +
> + lcd0: lcd-controller at 01c0c000 {
> + compatible = "allwinner,sun8i-a83t-tcon";
> + ...

ditto.

> + clocks = < CLK_BUS_TCON0>, < CLK_TCON0>;
> + clock-names = "gate", "clock";
> + resets = < RST_BUS_TCON0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + lcd0_p: port {
> + lcd0_ep: endpoint {
> + remote-endpoint = <_ep>;
> + };
> + };
> + };
> +
> + hdmi: hdmi at 01ee {
> + ...
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port {
> + hdmi_ep: endpoint {
> + remote-endpoint = <_ep>;
> + };
> + };
> + };
> +

[PATCH] drm/fences: add DOC: for explicit fencing

[Daniel Vetter]

On Mon, Nov 21, 2016 at 12:48:13PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Document IN_FENCE_FD and OUT_FENCE_PTR properties.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++
>  drivers/gpu/drm/drm_atomic.c  | 31 +++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 568f3c2..cdc9539 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -287,6 +287,12 @@ Tile Group Property
>  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :doc: Tile group
>  
> +Explicit Fencing Properties
> +---
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: explicit fencing properties
> +
>  Existing KMS Properties
>  ---
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b476ec5..7f33031 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1809,6 +1809,37 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +/**
> + * DOC: explicit fencing properties
> + *
> + * Explicit fencing allows userspace to control the buffer synchronization
> + * between devices. A Fence or a group of fences are trasnfered to/from

s/trasn/trans/

> + * userspace using Sync File fds and there are two DRM properties for that.
> + * IN_FENCE_FD on each DRM Plane to send fences to the kernel and
> + * OUT_FENCE_PTR on each DRM CRTC to receive fences from the kernel.

I think a bit about implicit vs. explicit fencing would be useful in a
separate paragraph:

"As a contrast, with implicit fencing the kernel keeps track of any
ongoing rendering, and automatically ensures that the atomic update waits
for any pending rendering to complete. For shared buffers represented with
a struct _buf this is tracked in _object structures.
Implicit syncing is how Linux traditionally worked (e.g. DRI2/3 on X.org),
whereas explicit fencing is what Android wants."

> + *
> + * "IN_FENCE_FD”:
> + *   Use this property to pass a fence that DRM should wait on before
> + *   proceeding with the Atomic Commit request and show the framebuffer for
> + *   the plane on the screen. The fence can be either a normal fence or a
> + *   merged one, the sync_file framework will handle both case and use a
s/case/cases/

> + *   fence_array if a merged fence is received. Passing -1 here means no
> + *   fences to wait on.

Please also document what the expecation is for a TEST_ONLY commit.

I think a line or so about the driver interface would be good here, e.g.

"On the driver side the fence is stored @fence parameter of struct
_plane_state. Drivers which also support implicit fencing should set
the implicit fence using drm_atomic_set_fence_for_plane(), to make sure
there's consistent behaviour between drivers in precedence of implicit vs.
explicit fencing."

> + *
> + * "OUT_FENCE_PTR”:
> + *   Use this property to pass a file descriptor pointer to DRM. Once the
> + *   Atomic Commit request call returns OUT_FENCE_PTR will be filled with
> + *   the file descriptor number of a Sync File. This Sync File contains the
> + *   CRTC fence that will be signaled when all framebuffers present on the
> + *   Atomic Commit * request for that given CRTC are scanned out on the
> + *   screen.
> + *
> + *   The Atomic Commit request fails if a invalid pointer is passed. If the
> + *   Atomic Commit request fails for any other reason the out fence fd
> + *   returned will be -1. On a Atomic Commit with the
> + *   DRM_MODE_ATOMIC_TEST_ONLY flag the out fence will also be set to -1.

Same here about driver interface:

"Note that out-fences don't have a special interface to drivers and are
internally represented by a struct _pending_vblank_event in struct
_crtc_state, which is also used by the async atomic commit helpers and
for the DRM event handling for existing userspace."

Cheers, Daniel

> + */
> +
>  static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc)
>  {
>   struct dma_fence *fence;
> -- 
> 2.5.5
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 03/13] devicetree/bindings: display: Add bindings for two Mitsubishi panels

[Rob Herring]

On Sat, Nov 19, 2016 at 05:28:03AM +0200, Laurent Pinchart wrote:
> The AA104XD12 and AA121TD01 are LVDS display panels. Their bindings are
> modelled on the the LVS panel bindings.

s/LVS/LVDS/

> 
> Signed-off-by: Laurent Pinchart 
> ---
>  .../display/panel/mitsubishi,aa104xd12.txt | 47 
> ++
>  .../display/panel/mitsubishi,aa121td01.txt | 47 
> ++
>  2 files changed, 94 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/mitsubishi,aa104xd12.txt
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/mitsubishi,aa121td01.txt

With that,

Acked-by: Rob Herring 

[PATCH v2 02/13] devicetree/bindings: display: Add bindings for LVDS panels

[Rob Herring]

On Sat, Nov 19, 2016 at 05:28:02AM +0200, Laurent Pinchart wrote:
> LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A.
> Multiple incompatible data link layers have been used over time to
> transmit image data to LVDS panels. This binding supports display panels
> compatible with the JEIDA-59-1999, Open-LDI and VESA SWPG
> specifications.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  .../bindings/display/panel/panel-lvds.txt  | 120 
> +
>  1 file changed, 120 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/panel-lvds.txt

Acked-by: Rob Herring 

[PATCH v2 01/13] devicetree/bindings: display: Document common panel properties

[Rob Herring]

On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote:
> Document properties common to several display panels in a central
> location that can be referenced by the panel device tree bindings.
> 

Looks good. Just one comment...

[...]

> +Connectivity
> +
> +
> +- ports: Panels receive video data through one or multiple connections. While
> +  the nature of those connections is specific to the panel type, the
> +  connectivity is expressed in a standard fashion using ports as specified in
> +  the device graph bindings defined in
> +  Documentation/devicetree/bindings/graph.txt.

We allow panels to either use graph binding or be a child of the display 
controller. Using the graph is preferred, but in the simple cases just a 
child node is sufficient. This should be described here or somewhere in 
this doc.

Rob

[BUG] hdlcd gets confused about base address

[Daniel Vetter]

On Fri, Nov 18, 2016 at 11:37:33PM +, 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 
> ---
> 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 0xf500 - 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Daniel Vetter]

On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > >>> if connector link status property has changed. This will tell the
> > > > > >>> driver to do a complete modeset due to change in connector 
> > > > > >>> property.
> > > > > >>>
> > > > > >>> Acked-by: Harry Wentland 
> > > > > >>> Acked-by: Tony Cheng 
> > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > >>> Cc: Jani Nikula 
> > > > > >>> Cc: Daniel Vetter 
> > > > > >>> Cc: Ville Syrjala 
> > > > > >>> Signed-off-by: Manasi Navare 
> > > > > >>> ---
> > > > > >>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > >>>  1 file changed, 7 insertions(+)
> > > > > >>>
> > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> index 0b16587..2125fd1 100644
> > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> @@ -519,6 +519,13 @@ static int 
> > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > >>>  connector_state);
> > > > > >>>   if (ret)
> > > > > >>>   return ret;
> > > > > >>> +
> > > > > >>> + if (connector->state->crtc) {
> > > > > >>> + crtc_state = 
> > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > >>> + 
> > > > > >>> connector->state->crtc);
> > > > > >>> + if (connector->link_status == 
> > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > >>> + crtc_state->connectors_changed = true;
> > > > > >>> + }
> > > > > >>>   }
> > > > > >>>  
> > > > > >>>   /*
> > > > > >> This will cause ordinary atomic commits that happen to change 
> > > > > >> connector flags to potentially fail with -EINVAL if ALLOW_MODESET 
> > > > > >> is not set.
> > > > > >> For this reason I'm not sure this flag should be set automatically 
> > > > > >> by the kernel. Could we add add a retrain link property instead, 
> > > > > >> that
> > > > > >> always return 0 when queried, but writing a 1 causing 
> > > > > >> connectors_changed to be set on bad link status?
> > > > > > Or just check for allow_modeset before setting 
> > > > > > connectors_changed=true here?
> > > > > 
> > > > > I don't think modesets should be done automatically like that, even 
> > > > > if ALLOW_MODESET is set a modeset may not be expected by userspace.
> > > > 
> > > > Presumably userspace would want a picture on the screen using any means
> > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> > > > probably say as much?
> > > 
> > > Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > screen.
> > > 
> > > What we could try is to gate this of whether userspace touches the mode
> > > property on the corresponding CRTC. I.e. if that's touched (even if it's
> > > the same mode), and a link is bad in one of the connectors in the state
> > > then we do a full modeset to recover.
> > > 
> > > Another option would be to make the link status writeable. Trying to
> > > change it from bad->good would force the modeset. That would be 100% clear
> > > to userspace, not special hacks needed with checking for allow_modeset,
> > > no magic property that auto-changes its value. And 100% backwards compat
> > > because existing userspace should never touch properties it doesn't
> > > understand (except when restoring a mode, and then it should allow a full
> > > modeset). And if someone does try a good->bad transition, we just silently
> > > keep it at good.
> > > 
> > > Definitely need to document this properly in the property docs, no matter
> > > what we decide.
> > 
> > Hmm. I think I kinda like this idea of userspace clear the state back
> > to good explicitly, if it happens with the same prop. So it's like
> > Maarten's retrain_link prop idea, but without having to add the second
> > prop to the mix.
> > 
> > It would also save me from pointing out (for the nth time) that the
> > link status should really be cleared to good during the commit state
> > swap and not at some random point during the commit ;)
> >
> 
> Okay, so change 1 is to make the userspace clear the state back to Good for 
> the property..
> Then Change 2 is to set