[Intel-gfx] [RFC 0/4] PMIC based Panel and Backlight Control

2014-12-26 Thread Shobhit Kumar 
Hi All,
For quite some time this problem is there in front of us and I am trying to have
a closure on how to go about doing this. This patch set is a crude 
implementation
to trigger the discussion. 

Daniel sometime back suggested to look at drm_panel for doing this but I was 
not able
to figure out how the panel driver will be loaded because PMIC probe success 
does not
really imply that we need to load a PMIC based panel driver. It all depends on 
the OEM
design if there is a DSI panel. Even then there is a possibility of SoC being 
usde for
panel control. All this decission can be made from the VBT information. Hence 
as of now
I have kept this inside i915 to init, if from VBT we know that PMIC controls 
the panel
enable/disable. Maybe drm_panel is not needed and we can have it internal to 
i915.

Similar problem is there for backlight control over PMIC as well. For that 
probably we
need to have independant backlight class driver, but this panel driver can also 
control
backlight if needed.

Also currently this needs PMIC register read/write capability which is not 
there in
upstream yet. For my testing I used the crude implementation for the same from 
a patch 
attached in -
https://bugs.freedesktop.org/show_bug.cgi?id=85977

Requesting your comments to converge on best way of implementing this.

Regards
Shobhit

Shobhit Kumar (4):
  drm/i915: Define a common data structure for Panel Info
  drm/i915: Add a drm_panel over INTEL_SOC_PMIC
  drm/i915/Kconfig: By default select DRM_PANEL
  drm/i915: Enable PMIC panel control as drm_panel for DSI

 drivers/gpu/drm/i915/Kconfig   |   1 +
 drivers/gpu/drm/i915/Makefile  |   1 +
 drivers/gpu/drm/i915/intel_drv.h   |  14 +++
 drivers/gpu/drm/i915/intel_dsi.c   |  20 +++-
 drivers/gpu/drm/i915/intel_dsi.h   |  13 +--
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  11 +-
 drivers/gpu/drm/i915/intel_panel_pmic.c| 157 +
 7 files changed, 202 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_panel_pmic.c

-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC 2/4] drm/i915: Add a drm_panel over INTEL_SOC_PMIC

2014-12-26 Thread Shobhit Kumar 
This driver just add PANEL_ENABLE/DISABLE control using drm_panel
framework.

Signed-off-by: Shobhit Kumar 
---
 drivers/gpu/drm/i915/Makefile   |   1 +
 drivers/gpu/drm/i915/intel_drv.h|   3 +
 drivers/gpu/drm/i915/intel_panel_pmic.c | 157 
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_panel_pmic.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 1849ffa..d282476 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -79,6 +79,7 @@ i915-y += dvo_ch7017.o \
  intel_i2c.o \
  intel_lvds.o \
  intel_panel.o \
+ intel_panel_pmic.o \
  intel_sdvo.o \
  intel_tv.o
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0088f16..1dab753 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1142,6 +1142,9 @@ extern struct drm_display_mode 
*intel_find_panel_downclock(
 void intel_backlight_register(struct drm_device *dev);
 void intel_backlight_unregister(struct drm_device *dev);
 
+/* intel_panel_soc_pmic.c */
+struct drm_panel *intel_panel_pmic_init(struct device *dev,
+   struct panel_info *info);
 
 /* intel_psr.c */
 void intel_psr_enable(struct intel_dp *intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_panel_pmic.c 
b/drivers/gpu/drm/i915/intel_panel_pmic.c
new file mode 100644
index 000..28a9f00
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_panel_pmic.c
@@ -0,0 +1,157 @@
+/*
+ * Copyright © 2006-2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ * Shobhit Kumar 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include "intel_drv.h"
+
+#define PMIC_PANEL_EN  0x52
+struct panel_pmic {
+   struct drm_panel base;
+   bool prepared;
+   bool enabled;
+
+   /* Panel enable/disable specififc delays */
+   struct panel_info pinfo;
+};
+
+static inline struct panel_pmic *to_panel_pmic(struct drm_panel *panel)
+{
+   return container_of(panel, struct panel_pmic, base);
+}
+
+static int panel_pmic_disable(struct drm_panel *panel)
+{
+   struct panel_pmic *p = to_panel_pmic(panel);
+
+   if (!p->enabled)
+   return 0;
+
+   DRM_DEBUG_KMS("\n");
+
+   /* invoke the pmic driver */
+   intel_soc_pmic_writeb(PMIC_PANEL_EN, 0x00);
+   msleep(p->pinfo.panel_off_delay);
+   msleep(p->pinfo.panel_pwr_cycle_delay);
+
+   p->enabled = false;
+
+   return 0;
+}
+
+static int panel_pmic_unprepare(struct drm_panel *panel)
+{
+   struct panel_pmic *p = to_panel_pmic(panel);
+
+   if (!p->prepared)
+   return 0;
+
+   /* Nothing needed */
+   p->prepared = false;
+
+   return 0;
+}
+
+static int panel_pmic_prepare(struct drm_panel *panel)
+{
+   struct panel_pmic *p = to_panel_pmic(panel);
+
+   if (p->prepared)
+   return 0;
+
+   /* Nothing needed */
+   p->prepared = true;
+
+   return 0;
+}
+
+static int panel_pmic_enable(struct drm_panel *panel)
+{
+   struct panel_pmic *p = to_panel_pmic(panel);
+
+   if (p->enabled)
+   return 0;
+
+   DRM_DEBUG_KMS("\n");
+
+   /* invoke the pmic driver */
+   intel_soc_pmic_writeb(PMIC_PANEL_EN, 0x01);
+   msleep(p->pinfo.panel_on_delay);
+
+   p->enabled = true;
+
+   return 0;
+}
+
+static const struct drm_panel_funcs panel_pmic_funcs = {
+   .disable = panel_pmic_disable,
+   .unprepare = panel_pmic_unprepare,
+   .prepare = panel_pmic_prepare,
+   .enable = panel_pmic_enable,
+};
+
+struct drm_panel *intel_panel_pmic_init(struct device *dev,
+   struct panel_info *info)
+{
+   struct panel_pmic *panel;
+   bool status = false;
+
+   DRM_DEBUG_

[Intel-gfx] [RFC 1/4] drm/i915: Define a common data structure for Panel Info

2014-12-26 Thread Shobhit Kumar 
As of now this includes only PPS and BLC delays. New things can be added
as and when needed

Signed-off-by: Shobhit Kumar 
---
 drivers/gpu/drm/i915/intel_drv.h   | 11 +++
 drivers/gpu/drm/i915/intel_dsi.h   |  7 +--
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 10 +-
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 588b618..0088f16 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -731,6 +731,17 @@ struct intel_load_detect_pipe {
int dpms_mode;
 };
 
+struct panel_info {
+   /* all delays in ms */
+   u16 backlight_off_delay;
+   u16 backlight_on_delay;
+   u16 panel_on_delay;
+   u16 panel_off_delay;
+   u16 panel_pwr_cycle_delay;
+
+   /* Other panel specififc stuff */
+};
+
 static inline struct intel_encoder *
 intel_attached_encoder(struct drm_connector *connector)
 {
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 8fe2064..8b20f76 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -129,12 +129,7 @@ struct intel_dsi {
u32 pclk;
u16 burst_mode_ratio;
 
-   /* all delays in ms */
-   u16 backlight_off_delay;
-   u16 backlight_on_delay;
-   u16 panel_on_delay;
-   u16 panel_off_delay;
-   u16 panel_pwr_cycle_delay;
+   struct panel_info pinfo;
 };
 
 /* XXX: Transitional before dual port configuration */
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c 
b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 5493aef..56b5b80 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -550,11 +550,11 @@ static bool generic_init(struct intel_dsi_device *dsi)
/* delays in VBT are in unit of 100us, so need to convert
 * here in ms
 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
-   intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
-   intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
-   intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
-   intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
-   intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
+   intel_dsi->pinfo.backlight_off_delay = pps->bl_disable_delay / 10;
+   intel_dsi->pinfo.backlight_on_delay = pps->bl_enable_delay / 10;
+   intel_dsi->pinfo.panel_on_delay = pps->panel_on_delay / 10;
+   intel_dsi->pinfo.panel_off_delay = pps->panel_off_delay / 10;
+   intel_dsi->pinfo.panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 
10;
 
return true;
 }
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC 3/4] drm/i915/Kconfig: By default select DRM_PANEL

2014-12-26 Thread Shobhit Kumar 
This will be needed for enabling drm_panel driver over pmic

Signed-off-by: Shobhit Kumar 
---
 drivers/gpu/drm/i915/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4e39ab3..3210dbb 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -18,6 +18,7 @@ config DRM_I915
select INPUT if ACPI
select ACPI_VIDEO if ACPI
select ACPI_BUTTON if ACPI
+   select DRM_PANEL
help
  Choose this option if you have a system that has "Intel Graphics
  Media Accelerator" or "HD Graphics" integrated graphics,
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC 4/4] drm/i915: Enable PMIC panel control as drm_panel for DSI

2014-12-26 Thread Shobhit Kumar 
This initialize the drm_panel based on PMIC driver and uses drm_panel_*
for controlling the panel enable/disable states

Signed-off-by: Shobhit Kumar 
---
 drivers/gpu/drm/i915/intel_dsi.c   | 20 
 drivers/gpu/drm/i915/intel_dsi.h   |  6 ++
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 42b6d6f..3a56674 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "i915_drv.h"
@@ -230,6 +231,8 @@ static void intel_dsi_pre_enable(struct intel_encoder 
*encoder)
 
DRM_DEBUG_KMS("\n");
 
+   drm_panel_enable(intel_dsi->panel);
+
/* Disable DPOunit clock gating, can stall pipe
 * and we need DPLL REFA always enabled */
tmp = I915_READ(DPLL(pipe));
@@ -247,8 +250,6 @@ static void intel_dsi_pre_enable(struct intel_encoder 
*encoder)
/* put device in ready state */
intel_dsi_device_ready(encoder);
 
-   msleep(intel_dsi->panel_on_delay);
-
if (intel_dsi->dev.dev_ops->panel_reset)
intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
 
@@ -390,8 +391,7 @@ static void intel_dsi_post_disable(struct intel_encoder 
*encoder)
if (intel_dsi->dev.dev_ops->disable_panel_power)
intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
 
-   msleep(intel_dsi->panel_off_delay);
-   msleep(intel_dsi->panel_pwr_cycle_delay);
+   drm_panel_disable(intel_dsi->panel);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
@@ -896,6 +896,18 @@ void intel_dsi_init(struct drm_device *dev)
fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
 
+   /* Initialize the PMIC based drm_panel if available on the platform */
+   if (intel_dsi->pps_blc == PPS_BLC_PMIC) {
+   intel_dsi->panel = intel_panel_pmic_init(dev->dev,
+   &intel_dsi->pinfo);
+   if (!intel_dsi->panel) {
+   DRM_ERROR("Panel init fail !! EN/DISABLE will not work 
leaking power !!\n");
+   return;
+   }
+
+   drm_panel_attach(intel_dsi->panel, connector);
+   }
+
return;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 8b20f76..641c80d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -33,6 +33,9 @@
 #define DSI_DUAL_LINK_FRONT_BACK   1
 #define DSI_DUAL_LINK_PIXEL_ALT2
 
+#define PPS_BLC_PMIC   0
+#define PPS_BLC_SOC1
+
 struct intel_dsi_device {
unsigned int panel_id;
const char *name;
@@ -83,6 +86,8 @@ struct intel_dsi {
 
struct intel_connector *attached_connector;
 
+   struct drm_panel *panel;
+
/* bit mask of ports being driven */
u16 ports;
 
@@ -116,6 +121,7 @@ struct intel_dsi {
u32 dphy_reg;
u32 video_frmt_cfg_bits;
u16 lp_byte_clk;
+   u8 pps_blc;
 
/* timeouts in byte clocks */
u16 lp_rx_timeout;
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c 
b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 56b5b80..b91667b 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -297,6 +297,7 @@ static bool generic_init(struct intel_dsi_device *dsi)
intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
intel_dsi->dual_link = mipi_config->dual_link;
intel_dsi->pixel_overlap = mipi_config->pixel_overlap;
+   intel_dsi->pps_blc = mipi_config->pwm_blc;
 
if (intel_dsi->dual_link)
intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C));
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] igt: Correct the return value for drm short_buffer read

2014-12-26 Thread Chris Wilson 
On Fri, Dec 26, 2014 at 01:16:06AM +, Zhang, Xiong Y wrote:
> > -Original Message-
> > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > Sent: Tuesday, December 23, 2014 7:31 PM
> > To: Zhang, Xiong Y
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] igt: Correct the return value for drm
> > short_buffer read
> > 
> > On Tue, Dec 23, 2014 at 10:14:15AM +, Zhang, Xiong Y wrote:
> > > > -Original Message-
> > > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > > > Sent: Tuesday, December 23, 2014 5:53 PM
> > > > To: Zhang, Xiong Y
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Subject: Re: [Intel-gfx] [PATCH] igt: Correct the return value for
> > > > drm short_buffer read
> > > >
> > > > On Tue, Dec 23, 2014 at 03:52:11PM +0800, Xiong Zhang wrote:
> > > > > After i915 commit:
> > > > > commit bd008e5b2953186fc0c6633a885ade95e7043800
> > > > > Author: Chris Wilson 
> > > > > Date:   Tue Oct 7 14:13:51 2014 +0100
> > > > >
> > > > > drm: Implement O_NONBLOCK support on /dev/dri/cardN
> > > > >
> > > > > the return value for drm short_buffer read is -1 and errno is EAGAIN.
> > > >
> > > > No, it is not.
> > > > -Chris
> > > Without this patch, system fail in short-buffer-block and
> > short-buffer-nonblock subtest.
> > > With this patch, these two subtest could pass.
> > 
> > That's the point of the test, the kernel behaviour is wrong. There is a 
> > patch to fix
> > the kernel.
> > -Chris
> [Zhang, Xiong Y] Oh, I know it. Thanks.
> So could you send this patch to fix it ?

http://patchwork.freedesktop.org/patch/38174/

Daniel wanted the igt test first...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

2014-12-26 Thread Paulo Zanoni 
2014-12-25 8:16 GMT-02:00 Chris Wilson :
> On Tue, Dec 23, 2014 at 10:35:38AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> We have dev_priv->fbc.size which is supposed to contain the compressed
>> FB size, but it is not: at find_compression_threshold() we try to
>> overallocate the CFB, but we don't consider this when we assign a
>> value to dev_priv->fbc.size. Since the correct CFB size should already
>> be stored at dev_priv->fbc.compressed_fb.size, just kill
>> dev_priv->fbc.size and use the correct value isntead.
>
> They should not be equivalent though. We actually want fbc.size to
> compensate for the compression in the allocation so that the simple
> check for enough space succeeds even if we are compressing.

Can you please elaborate more on that? Are you talking about the
threshold? As far as I can see, we're failing to properly take it into
consideration both before and after this patch, so it wouldn't be a
valid reason.

I was planning to fix the "take threshold into consideration when
checking the size" problem later: I wanted to think on a way that
would guarantee that we'd always get the best possible threshold to
prevent cases where we'd keep reusing 1:4 compression even while we
could just free+realloc to have 1:1 compression back.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

2014-12-26 Thread Paulo Zanoni 
2014-12-25 8:20 GMT-02:00 Chris Wilson :
> On Tue, Dec 23, 2014 at 10:35:40AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> Because that is probably not very a good idea: if we used the stolen
>> memory for more things, there could be a risk that someone would
>> "allocate" the memory that the HW is still using as the CFB while FBC
>> was still enabled.
>
> Not in the code as it currently is due to the struct mutex lock. You
> need to explain how and why you intend to remove that invariant.

Ok, maybe this point is wrong, but I still wee we freeing and then
reallocating the CFB while FBC is active, which is still a problem,
even if we end up just reallocating the same chunk of memory later.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: add the FBC mutex

2014-12-26 Thread Paulo Zanoni 
2014-12-25 8:25 GMT-02:00 Chris Wilson :
> On Tue, Dec 23, 2014 at 10:35:42AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> Make sure we're not gonna have weird races in really weird cases where
>> a lot of different CRTCs are doing rendering and modesets at the same
>> time.
>
> You are not actually reducing the partial coverage from other locks,
> particularly struct_mutex, so this doesn't look like the full improvement
> it should be.

Good point. I'll take a look at that.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

2014-12-26 Thread Paulo Zanoni 
2014-12-25 8:33 GMT-02:00 Chris Wilson :
> On Tue, Dec 23, 2014 at 10:35:43AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> Because there's no need for it. Just use a static structure with a
>> bool field to tell if it's in use or not. The big advantage here is
>> not saving kzalloc/kfree calls, it's cutting the ugly "failed to
>> allocate FBC work structure" code path: in this path we call
>> enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
>> fbc.y - they are updated in intel_fbc_work_fn(), which we're not
>> calling. And since testing out-of-memory cases like this is really
>> hard, getting rid of the code path is a major relief.
>
> No, it is not that hard to test.

Yeah, I know we have tons of checks for out-of-memory stuff, but it
takes time to test and I think we just don't have the FBC-specific
test.

>
> The complaint here should be addressed by a function to call
> dev_priv->display.enable_fbc, which would do the common task of setting
> dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

Ok, I'll do that. I thought that by keeping a single code path we'd be
able to avoid it :)

>
> That would remove duplicated code first. And then you can argue about
> the merits of replacing the kmalloc by growing our global state.

And what is your opinion on this? Do you think it's an improvement to
the codebase?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

2014-12-26 Thread Chris Wilson 
On Fri, Dec 26, 2014 at 11:46:34AM -0200, Paulo Zanoni wrote:
> 2014-12-25 8:16 GMT-02:00 Chris Wilson :
> > On Tue, Dec 23, 2014 at 10:35:38AM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni 
> >>
> >> We have dev_priv->fbc.size which is supposed to contain the compressed
> >> FB size, but it is not: at find_compression_threshold() we try to
> >> overallocate the CFB, but we don't consider this when we assign a
> >> value to dev_priv->fbc.size. Since the correct CFB size should already
> >> be stored at dev_priv->fbc.compressed_fb.size, just kill
> >> dev_priv->fbc.size and use the correct value isntead.
> >
> > They should not be equivalent though. We actually want fbc.size to
> > compensate for the compression in the allocation so that the simple
> > check for enough space succeeds even if we are compressing.
> 
> Can you please elaborate more on that? Are you talking about the
> threshold? As far as I can see, we're failing to properly take it into
> consideration both before and after this patch, so it wouldn't be a
> valid reason.

We have both identified the problem, and the fix to the current code
looks rather easy as it seems a simple bug in

commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
Author: Ben Widawsky 
Date:   Mon Jun 30 10:41:24 2014 -0700

drm/i915: Try harder to get FBC

that fluffs the compare of uncompressed request sizes.

Your subject line claims to be a fix, so fix something.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

2014-12-26 Thread Chris Wilson 
On Fri, Dec 26, 2014 at 11:46:49AM -0200, Paulo Zanoni wrote:
> 2014-12-25 8:20 GMT-02:00 Chris Wilson :
> > On Tue, Dec 23, 2014 at 10:35:40AM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni 
> >>
> >> Because that is probably not very a good idea: if we used the stolen
> >> memory for more things, there could be a risk that someone would
> >> "allocate" the memory that the HW is still using as the CFB while FBC
> >> was still enabled.
> >
> > Not in the code as it currently is due to the struct mutex lock. You
> > need to explain how and why you intend to remove that invariant.
> 
> Ok, maybe this point is wrong, but I still wee we freeing and then
> reallocating the CFB while FBC is active, which is still a problem,
> even if we end up just reallocating the same chunk of memory later.

No. If you read the current code it will disable FBC using the old
location before it can be reallocated. That it may continue to use the
old address for a frame after we have marked it free isn't clean, but it
is not the bug you describe.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 3.19-rc1 errors when opening LID

2014-12-26 Thread Rafael J. Wysocki 
On Wednesday, December 24, 2014 07:51:48 PM Pali Rohár wrote:
> 
> --nextPart5943893.pKyMBm5Emp
> Content-Type: Text/Plain;
>   charset="utf-8"
> Content-Transfer-Encoding: quoted-printable
> 
> Hello!
> 
> With new 3.19-rc1 kernel every time when I open LID on my laptop, kernel pr=
> ints these error lines to dmesg:

Does your syste suspend when the lid is closed and resume when it is open?

> [25566.368133] [drm:hsw_unclaimed_reg_detect.isra.6 [i915]] *ERROR* Unclaim=
> ed register detected. Please use the i915.mmio_debug=3D1 to debug this prob=
> lem.
> [25566.368134] [drm:intel_uncore_check_errors [i915]] *ERROR* Unclaimed reg=
> ister before interrupt
> [25566.368192] [drm:hsw_unclaimed_reg_detect.isra.6 [i915]] *ERROR* Unclaim=
> ed register detected. Please use the i915.mmio_debug=3D1 to debug this prob=
> lem.
> [25566.368232] [drm:hsw_unclaimed_reg_detect.isra.6 [i915]] *ERROR* Unclaim=
> ed register detected. Please use the i915.mmio_debug=3D1 to debug this prob=
> lem.<4>[25568.446011] i915 :00:02.0: BAR 6: [??? 0x flags 0x2]=
> =20
> has bogus alignment

The above messages are from i915 and don't seem to be related to ACPI.

> [25568.447018] pci_bus :02: Allocating resources
> [25568.447055] pci_bus :03: Allocating resources
> [25568.447135] pci_bus :04: Allocating resources
> [25568.447168] pci_bus :05: Allocating resources
> [25568.447195] pci_bus :06: Allocating resources
> [25568.447228] pci_bus :0e: Allocating resources
> [25568.447323] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.447557] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.447735] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.447847] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.448399] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.448438] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment

The above too.

> [25568.448566] acpi MSFT:00: Cannot transition to power state D3cold fo=
> r parent in (unknown)
> [25568.448572] acpi INT33C2:00: Cannot transition to non-D0 state from D3
> [25568.448577] acpi MSFT0002:00: Cannot transition to power state D3cold fo=
> r parent in (unknown)
> [25568.448581] acpi ELAN1010:00: Cannot transition to power state D3cold fo=
> r parent in (unknown)
> [25568.448587] acpi INT33C3:00: Cannot transition to non-D0 state from D3
> [25568.448590] acpi INT33C0:00: Cannot transition to non-D0 state from D3
> [25568.448594] acpi INT33C1:00: Cannot transition to non-D0 state from D3
> [25568.448598] acpi INT33C4:00: Cannot transition to non-D0 state from D3
> [25568.448602] acpi INT33C5:00: Cannot transition to non-D0 state from D3
> [25568.448623] acpi device:41: Cannot transition to power state D3cold for =
> parent in (unknown)
> [25568.448627] acpi INT33C6:00: Cannot transition to non-D0 state from D3

All of the above mean that power transitions were not carried out for some
devices, because they were in unexpected power states to start with.

They are devices on the Intel LPSS as far as I can say (CCing Mika and Andy).

> [25568.448890] pci_bus :01: Allocating resources
> [25568.448905] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.449064] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> [25568.449472] i915 :00:02.0: BAR 6: [??? 0x flags 0x2] has bog=
> us alignment
> 
> With older kernel (3.13) I do not see these errors, so it is regression.

It only is a regression if it leads to functional problems.  Do you see any?

> Ca=n you look at it? If it is needed, I can provide other logs, ACPI/DSTD 
> dump=
> s, etc.

It looks like 3.13 didn't try to use some devices that are now used in 3.19-rc1
and something doesn't work entirely as expected.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 3.19-rc1 errors when opening LID

2014-12-26 Thread Pali Rohár 
On Friday 26 December 2014 16:12:49 Rafael J. Wysocki wrote:
> On Wednesday, December 24, 2014 07:51:48 PM Pali Rohár wrote:
> > --nextPart5943893.pKyMBm5Emp
> > Content-Type: Text/Plain;
> > 
> >   charset="utf-8"
> > 
> > Content-Transfer-Encoding: quoted-printable
> > 
> > Hello!
> > 
> > With new 3.19-rc1 kernel every time when I open LID on my
> > laptop, kernel pr=
> 
> > ints these error lines to dmesg:
> Does your syste suspend when the lid is closed and resume when
> it is open?
> 

No, I disabled all autosuspend and autohibernate settings. When I 
close LID system is running.

> 
> > [25568.448566] acpi MSFT:00: Cannot transition to power
> > state D3cold fo= r parent in (unknown)
> > [25568.448572] acpi INT33C2:00: Cannot transition to non-D0
> > state from D3 [25568.448577] acpi MSFT0002:00: Cannot
> > transition to power state D3cold fo= r parent in (unknown)
> > [25568.448581] acpi ELAN1010:00: Cannot transition to power
> > state D3cold fo= r parent in (unknown)
> > [25568.448587] acpi INT33C3:00: Cannot transition to non-D0
> > state from D3 [25568.448590] acpi INT33C0:00: Cannot
> > transition to non-D0 state from D3 [25568.448594] acpi
> > INT33C1:00: Cannot transition to non-D0 state from D3
> > [25568.448598] acpi INT33C4:00: Cannot transition to non-D0
> > state from D3 [25568.448602] acpi INT33C5:00: Cannot
> > transition to non-D0 state from D3 [25568.448623] acpi
> > device:41: Cannot transition to power state D3cold for =
> > parent in (unknown)
> > [25568.448627] acpi INT33C6:00: Cannot transition to non-D0
> > state from D3
> 
> All of the above mean that power transitions were not carried
> out for some devices, because they were in unexpected power
> states to start with.
> 
> They are devices on the Intel LPSS as far as I can say (CCing
> Mika and Andy).
> 

Just to note: all above acpi devices do not exist in 3.13 kernel, 
at least I do not see them in /sys/bus/acpi/.

> > [25568.448890] pci_bus :01: Allocating resources
> > [25568.448905] i915 :00:02.0: BAR 6: [??? 0x
> > flags 0x2] has bog= us alignment
> > [25568.449064] i915 :00:02.0: BAR 6: [??? 0x
> > flags 0x2] has bog= us alignment
> > [25568.449472] i915 :00:02.0: BAR 6: [??? 0x
> > flags 0x2] has bog= us alignment
> > 
> > With older kernel (3.13) I do not see these errors, so it is
> > regression.
> 
> It only is a regression if it leads to functional problems. 
> Do you see any?
> 

Ok. I do not see functional problems, but I see above messages in 
dmesg log every time when I open LID.

> > Ca=n you look at it? If it is needed, I can provide other
> > logs, ACPI/DSTD dump= s, etc.
> 
> It looks like 3.13 didn't try to use some devices that are now
> used in 3.19-rc1 and something doesn't work entirely as
> expected.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires

2014-12-26 Thread Paulo Zanoni 
2014-12-18 10:47 GMT-02:00 Chris Wilson :
> On Thu, Dec 18, 2014 at 02:36:54PM +0200, Jani Nikula wrote:
>> On Thu, 18 Dec 2014, Chris Wilson  wrote:
>> > If we have a single unclaimed register, we will have lots. A WARN for
>> > each one makes the machine unusable and does not aid debugging. Convert
>> > the i915.mmio_debug option to a counter for how many WARNs to fire
>> > before shutting up. Even when i915.mmio_debug was disabled it would
>> > continue to shout an *ERROR* for every interrupt, without any
>> > information at all for debugging.
>> >
>> > The massive verbiage was added in
>> > commit 5978118c39c2f72fd8b39ef9c086723542384809
>> > Author: Paulo Zanoni 
>> > Date:   Wed Jul 16 17:49:29 2014 -0300
>> >
>> > drm/i915: reorganize the unclaimed register detection code
>> >
>> > v2: Automatically enable invalid mmio reporting for the *next* invalid
>> > access if mmio_debug is disabled by default. This should give us clearer
>> > debug information without polluting the logs too much.
>> > v3: Compile fixes, rebase.
>> >
>> > Signed-off-by: Chris Wilson 
>> > Cc: Rodrigo Vivi 
>> > Cc: Paulo Zanoni 
>> > Cc: Daniel Vetter 
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>> >  drivers/gpu/drm/i915/i915_params.c  |  6 +++---
>> >  drivers/gpu/drm/i915/intel_uncore.c | 10 --
>> >  3 files changed, 12 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> > b/drivers/gpu/drm/i915/i915_drv.h
>> > index 3047291ff2b9..ca9e21545063 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -2430,7 +2430,7 @@ struct i915_params {
>> > bool disable_display;
>> > bool disable_vtd_wa;
>> > int use_mmio_flip;
>> > -   bool mmio_debug;
>> > +   int mmio_debug;
>> > bool verbose_state_checks;
>> >  };
>> >  extern struct i915_params i915 __read_mostly;
>> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> > b/drivers/gpu/drm/i915/i915_params.c
>> > index 07252d8dc726..43c1df830531 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -170,10 +170,10 @@ module_param_named(use_mmio_flip, 
>> > i915.use_mmio_flip, int, 0600);
>> >  MODULE_PARM_DESC(use_mmio_flip,
>> >  "use MMIO flips (-1=never, 0=driver discretion [default], 
>> > 1=always)");
>> >
>> > -module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
>> > +module_param_named(mmio_debug, i915.mmio_debug, int, 0600);
>> >  MODULE_PARM_DESC(mmio_debug,
>> > -   "Enable the MMIO debug code (default: false). This may negatively "
>> > -   "affect performance.");
>> > +   "Enable the MMIO debug code (default: off). "
>> > +   "This may negatively affect performance.");
>>
>> Why not describe the new behaviour here instead of a comment in
>> intel_uncore.c?
>
> Secrets and wording.
> "Enable the MMIO debug code for the first N failures (default: off). "
>
>> >  module_param_named(verbose_state_checks, i915.verbose_state_checks, bool, 
>> > 0600);
>> >  MODULE_PARM_DESC(verbose_state_checks,
>> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> > b/drivers/gpu/drm/i915/intel_uncore.c
>> > index e9561de382aa..a3b662de1bdb 100644
>> > --- a/drivers/gpu/drm/i915/intel_uncore.c
>> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> > @@ -722,18 +722,24 @@ hsw_unclaimed_reg_debug(struct drm_i915_private 
>> > *dev_priv, u32 reg, bool read,
>> > WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
>> >  when, op, reg);
>> > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> > +   i915.mmio_debug--; /* Only report the first N failures */
>> > }
>> >  }
>> >
>> >  static void
>> >  hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> >  {
>> > -   if (i915.mmio_debug)
>> > +   static bool mmio_debug_once = true;
>> > +
>> > +   if (i915.mmio_debug || !mmio_debug_once)
>> > return;
>> >
>> > if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> > -   DRM_ERROR("Unclaimed register detected. Please use the 
>> > i915.mmio_debug=1 to debug this problem.");
>> > +   DRM_DEBUG("Unclaimed register detected, "
>> > + "enabling oneshot unclaimed register reporting. "
>> > + "Please use i915.mmio_debug=N for more 
>> > information.\n");


In addition to Jani's comments: I can already see people trying to use
a literal N instead of a number and reporting to us that it's broken.

With at least the improved MODULE_PARM_DESC text that you wrote in your reply:
Reviewed-by: Paulo Zanoni 



>> > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> > +   i915.mmio_debug = mmio_debug_once--;
>>
>> /me frowns upon the bool assignment to int and bool post-decrement. It's
>> not quite IOCCC but a demonstration of things that suck about C.
>>
>> Is it on purpose that, if you've set i915.mmio_debug=N, you first