[PATCH 0/2] Audio client support for VGA-switcheroo

[Takashi Iwai]

Dave,

this is a small patch series to add the support for audio clients
to VGA switcheroo.  The background problem is that the HD-audio HDMI
controller of the discrete GPU is also disabled when the graphics core
is disabled by vga-switcheroo.  This leads to a long stall of the
audio driver, or at worst, it Oopses.
  https://bugzilla.kernel.org/show_bug.cgi?id=43155

I tried to work around it only in the audio driver side, but it
doesn't seem to help.  When the graphics is disabled, the whole PCI
entry can't be accessed any longer.  Even reading a config triggers an
Oops.  So, the only option is to implement the VGA switcheroo client
for the audio.

The first patch refactors the vga-switcheroo code to be ready to
accept for more than two clients.  It's mostly a clean up using linked
lists.  The next patch implements the audio client registration.

BTW, if these patches are OK, the question remains how to merge.
Obviously I'd need more patches for HD-audio driver based on the new
vga_switcheroo_register_audio_client().  One option is to apply these
patches to a persistent branch (i.e. no rebase planned) of your tree
and pull it to my tree with more patches for HD-audio.
Does it sound good for you?


thanks,

Takashi

[PATCH 0/2] Audio client support for VGA-switcheroo

[Takashi Iwai]

Dave,

this is a small patch series to add the support for audio clients
to VGA switcheroo.  The background problem is that the HD-audio HDMI
controller of the discrete GPU is also disabled when the graphics core
is disabled by vga-switcheroo.  This leads to a long stall of the
audio driver, or at worst, it Oopses.
  https://bugzilla.kernel.org/show_bug.cgi?id=43155

I tried to work around it only in the audio driver side, but it
doesn't seem to help.  When the graphics is disabled, the whole PCI
entry can't be accessed any longer.  Even reading a config triggers an
Oops.  So, the only option is to implement the VGA switcheroo client
for the audio.

The first patch refactors the vga-switcheroo code to be ready to
accept for more than two clients.  It's mostly a clean up using linked
lists.  The next patch implements the audio client registration.

BTW, if these patches are OK, the question remains how to merge.
Obviously I'd need more patches for HD-audio driver based on the new
vga_switcheroo_register_audio_client().  One option is to apply these
patches to a persistent branch (i.e. no rebase planned) of your tree
and pull it to my tree with more patches for HD-audio.
Does it sound good for you?


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] vga_switcheroo: Refactor using linked list

[Takashi Iwai]

Refactor the code base a bit for the further work to adapt more clients.

Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/vga/vga_switcheroo.c |  209 --
 1 file changed, 110 insertions(+), 99 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 58434e8..c91573f 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -37,6 +37,7 @@ struct vga_switcheroo_client {
bool (*can_switch)(struct pci_dev *pdev);
int id;
bool active;
+   struct list_head list;
 };
 
 static DEFINE_MUTEX(vgasr_mutex);
@@ -51,7 +52,7 @@ struct vgasr_priv {
struct dentry *switch_file;
 
int registered_clients;
-   struct vga_switcheroo_client clients[VGA_SWITCHEROO_MAX_CLIENTS];
+   struct list_head clients;
 
struct vga_switcheroo_handler *handler;
 };
@@ -60,7 +61,9 @@ static int vga_switcheroo_debugfs_init(struct vgasr_priv 
*priv);
 static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);
 
 /* only one switcheroo per system */
-static struct vgasr_priv vgasr_priv;
+static struct vgasr_priv vgasr_priv = {
+   .clients = LIST_HEAD_INIT(vgasr_priv.clients),
+};
 
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler)
 {
@@ -86,17 +89,18 @@ EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
 
 static void vga_switcheroo_enable(void)
 {
-   int i;
int ret;
+   struct vga_switcheroo_client *client;
+
/* call the handler to init */
vgasr_priv.handler-init();
 
-   for (i = 0; i  VGA_SWITCHEROO_MAX_CLIENTS; i++) {
-   ret = 
vgasr_priv.handler-get_client_id(vgasr_priv.clients[i].pdev);
+   list_for_each_entry(client, vgasr_priv.clients, list) {
+   ret = vgasr_priv.handler-get_client_id(client-pdev);
if (ret  0)
return;
 
-   vgasr_priv.clients[i].id = ret;
+   client-id = ret;
}
vga_switcheroo_debugfs_init(vgasr_priv);
vgasr_priv.active = true;
@@ -107,28 +111,27 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
   void (*reprobe)(struct pci_dev *pdev),
   bool (*can_switch)(struct pci_dev *pdev))
 {
-   int index;
+   struct vga_switcheroo_client *client;
 
-   mutex_lock(vgasr_mutex);
-   /* don't do IGD vs DIS here */
-   if (vgasr_priv.registered_clients  1)
-   index = 1;
-   else
-   index = 0;
-
-   vgasr_priv.clients[index].pwr_state = VGA_SWITCHEROO_ON;
-   vgasr_priv.clients[index].pdev = pdev;
-   vgasr_priv.clients[index].set_gpu_state = set_gpu_state;
-   vgasr_priv.clients[index].reprobe = reprobe;
-   vgasr_priv.clients[index].can_switch = can_switch;
-   vgasr_priv.clients[index].id = -1;
+   client = kzalloc(sizeof(*client), GFP_KERNEL);
+   if (!client)
+   return -ENOMEM;
+
+   client-pwr_state = VGA_SWITCHEROO_ON;
+   client-pdev = pdev;
+   client-set_gpu_state = set_gpu_state;
+   client-reprobe = reprobe;
+   client-can_switch = can_switch;
+   client-id = -1;
if (pdev-resource[PCI_ROM_RESOURCE].flags  IORESOURCE_ROM_SHADOW)
-   vgasr_priv.clients[index].active = true;
+   client-active = true;
 
-   vgasr_priv.registered_clients |= (1  index);
+   mutex_lock(vgasr_mutex);
+   list_add_tail(client-list, vgasr_priv.clients);
+   vgasr_priv.registered_clients++;
 
/* if we get two clients + handler */
-   if (vgasr_priv.registered_clients == 0x3  vgasr_priv.handler) {
+   if (vgasr_priv.registered_clients == 2  vgasr_priv.handler) {
printk(KERN_INFO vga_switcheroo: enabled\n);
vga_switcheroo_enable();
}
@@ -137,18 +140,47 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL(vga_switcheroo_register_client);
 
+static struct vga_switcheroo_client *
+find_client_from_pci(struct list_head *head, struct pci_dev *pdev)
+{
+   struct vga_switcheroo_client *client;
+   list_for_each_entry(client, head, list)
+   if (client-pdev == pdev)
+   return client;
+   return NULL;
+}
+
+static struct vga_switcheroo_client *
+find_client_from_id(struct list_head *head, int client_id)
+{
+   struct vga_switcheroo_client *client;
+   list_for_each_entry(client, head, list)
+   if (client-id == client_id)
+   return client;
+   return NULL;
+}
+
+static struct vga_switcheroo_client *
+find_active_client(struct list_head *head)
+{
+   struct vga_switcheroo_client *client;
+   list_for_each_entry(client, head, list)
+   if (client-active == true)
+   return client;
+   return NULL;
+}
+
 void vga_switcheroo_unregister_client(struct

[PATCH 2/2] vga_switcheroo: Add the support for audio clients

[Takashi Iwai]

Add the support for audio clients to VGA-switcheroo for handling the
HDMI audio controller together with VGA switching.  The id of the
audio controller should be given explicity at registration time unlike
the video controller.

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

Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/vga/vga_switcheroo.c |   84 ++
 include/linux/vga_switcheroo.h   |   10 +
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index c91573f..df25ea6 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -57,6 +57,11 @@ struct vgasr_priv {
struct vga_switcheroo_handler *handler;
 };
 
+#define ID_BIT_AUDIO   0x100
+#define client_is_audio(c) ((c)-id  ID_BIT_AUDIO)
+#define client_is_vga(c)   ((c)-id == -1 || !client_is_audio(c))
+#define client_id(c)   ((c)-id  ~ID_BIT_AUDIO)
+
 static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);
 static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);
 
@@ -96,6 +101,8 @@ static void vga_switcheroo_enable(void)
vgasr_priv.handler-init();
 
list_for_each_entry(client, vgasr_priv.clients, list) {
+   if (client-id != -1)
+   continue;
ret = vgasr_priv.handler-get_client_id(client-pdev);
if (ret  0)
return;
@@ -106,10 +113,11 @@ static void vga_switcheroo_enable(void)
vgasr_priv.active = true;
 }
 
-int vga_switcheroo_register_client(struct pci_dev *pdev,
-  void (*set_gpu_state)(struct pci_dev *pdev, 
enum vga_switcheroo_state),
-  void (*reprobe)(struct pci_dev *pdev),
-  bool (*can_switch)(struct pci_dev *pdev))
+static int register_client(struct pci_dev *pdev,
+   void (*set_gpu_state)(struct pci_dev *pdev, enum vga_switcheroo_state),
+   void (*reprobe)(struct pci_dev *pdev),
+   bool (*can_switch)(struct pci_dev *pdev),
+   int id, bool active)
 {
struct vga_switcheroo_client *client;
 
@@ -122,24 +130,48 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
client-set_gpu_state = set_gpu_state;
client-reprobe = reprobe;
client-can_switch = can_switch;
-   client-id = -1;
-   if (pdev-resource[PCI_ROM_RESOURCE].flags  IORESOURCE_ROM_SHADOW)
-   client-active = true;
+   client-id = id;
+   client-active = active;
 
mutex_lock(vgasr_mutex);
list_add_tail(client-list, vgasr_priv.clients);
-   vgasr_priv.registered_clients++;
+   if (client_is_vga(client))
+   vgasr_priv.registered_clients++;
 
/* if we get two clients + handler */
-   if (vgasr_priv.registered_clients == 2  vgasr_priv.handler) {
+   if (!vgasr_priv.active 
+   vgasr_priv.registered_clients == 2  vgasr_priv.handler) {
printk(KERN_INFO vga_switcheroo: enabled\n);
vga_switcheroo_enable();
}
mutex_unlock(vgasr_mutex);
return 0;
 }
+
+int vga_switcheroo_register_client(struct pci_dev *pdev,
+  void (*set_gpu_state)(struct pci_dev *pdev, 
enum vga_switcheroo_state),
+  void (*reprobe)(struct pci_dev *pdev),
+  bool (*can_switch)(struct pci_dev *pdev))
+{
+   bool active = false;
+   if (pdev-resource[PCI_ROM_RESOURCE].flags  IORESOURCE_ROM_SHADOW)
+   active = true;
+   return register_client(pdev, set_gpu_state, reprobe, can_switch,
+  -1, active);
+}
 EXPORT_SYMBOL(vga_switcheroo_register_client);
 
+int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
+   void (*set_gpu_state)(struct pci_dev *pdev, enum vga_switcheroo_state),
+   void (*reprobe)(struct pci_dev *pdev),
+   bool (*can_switch)(struct pci_dev *pdev),
+   int id, bool active)
+{
+   return register_client(pdev, set_gpu_state, reprobe, can_switch,
+  id | ID_BIT_AUDIO, active);
+}
+EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
+
 static struct vga_switcheroo_client *
 find_client_from_pci(struct list_head *head, struct pci_dev *pdev)
 {
@@ -165,7 +197,7 @@ find_active_client(struct list_head *head)
 {
struct vga_switcheroo_client *client;
list_for_each_entry(client, head, list)
-   if (client-active == true)
+   if (client-active  client_is_vga(client))
return client;
return NULL;
 }
@@ -177,13 +209,16 @@ void vga_switcheroo_unregister_client(struct pci_dev 
*pdev)
mutex_lock(vgasr_mutex);
client = find_client_from_pci(vgasr_priv.clients, pdev);
if (client) {
+   if (client_is_vga

Re: [PATCH 0/2] Audio client support for VGA-switcheroo

[Takashi Iwai]

At Thu, 10 May 2012 09:08:38 +0200,
Takashi Iwai wrote:
 
 Dave,
 
 this is a small patch series to add the support for audio clients
 to VGA switcheroo.  The background problem is that the HD-audio HDMI
 controller of the discrete GPU is also disabled when the graphics core
 is disabled by vga-switcheroo.  This leads to a long stall of the
 audio driver, or at worst, it Oopses.
   https://bugzilla.kernel.org/show_bug.cgi?id=43155
 
 I tried to work around it only in the audio driver side, but it
 doesn't seem to help.  When the graphics is disabled, the whole PCI
 entry can't be accessed any longer.  Even reading a config triggers an
 Oops.  So, the only option is to implement the VGA switcheroo client
 for the audio.
 
 The first patch refactors the vga-switcheroo code to be ready to
 accept for more than two clients.  It's mostly a clean up using linked
 lists.  The next patch implements the audio client registration.
 
 BTW, if these patches are OK, the question remains how to merge.
 Obviously I'd need more patches for HD-audio driver based on the new
 vga_switcheroo_register_audio_client().  One option is to apply these
 patches to a persistent branch (i.e. no rebase planned) of your tree
 and pull it to my tree with more patches for HD-audio.
 Does it sound good for you?

Or, if you prefer, just pull from topic/vga-switcheroo branch of my tree:
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git 
topic/vga-switcheroo


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] vga_switcheroo: Add the support for audio clients

[Takashi Iwai]

At Thu, 10 May 2012 11:40:50 +0100,
Dave Airlie wrote:
 
 On Thu, May 10, 2012 at 8:10 AM, Takashi Iwai ti...@suse.de wrote:
  Add the support for audio clients to VGA-switcheroo for handling the
  HDMI audio controller together with VGA switching.  The id of the
  audio controller should be given explicity at registration time unlike
  the video controller.
 
 The only question I really have is how you assign the audio ids.
 
 Currently we mostly don't see  1 HDMI audio on a GPU, but that
 should be changing with the latest ones.

There is one HDMI audio controller entry usually per graphics.
There are more than one codecs can be connected to the controller,
e.g. Nvidia has up to 8 codecs, but this is an issue inside HD-audio
driver, and irrelevant from VGA-switcheroo.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] vga_switcheroo: Add the support for audio clients

[Takashi Iwai]

At Thu, 10 May 2012 13:42:09 +0200,
Takashi Iwai wrote:
 
 At Thu, 10 May 2012 12:20:05 +0100,
 Alan Cox wrote:
  
  On Thu, 10 May 2012 09:10:16 +0200
  Takashi Iwai ti...@suse.de wrote:
  
   Add the support for audio clients to VGA-switcheroo for handling the
   HDMI audio controller together with VGA switching.  The id of the
   audio controller should be given explicity at registration time unlike
   the video controller.
  
  It would I think be a lot cleaner and more future proof to pass a 
  
  struct hdmi_audio_switch_ops
  
  or some similar named object with an array of function pointers ?
 
 That would be a good option, indeed.
 
 In my patch series, I just didn't want to break the existing API,
 so I kept the current style.

Dave, do you prefer the way with an ops struct as Alan suggested?

For example, I can make like

struct vga_switcheroo_client_ops {
   void (*set_gpu_state)(struct pci_dev *dev, enum vga_switcheroo_state),
   void (*reprobe)(struct pci_dev *dev),
   bool (*can_switch)(struct pci_dev *dev));
};

and pass the pointer to vga_switcher_register_client() and
vga_switcher_register_audio_client().

If it's preferred, I'll work on it and resend you a pull request later.

BTW, I modified topic/vga-switcheroo branch of sound git tree again.
Now it contains only two commits I posted here.  The rest commits for
HD-audio are found in topic/hda-switcheroo branch.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] vga_switcheroo: Add the support for audio clients

[Takashi Iwai]

At Thu, 10 May 2012 20:05:25 +0100,
Dave Airlie wrote:
 
 On Thu, May 10, 2012 at 7:42 PM, Takashi Iwai ti...@suse.de wrote:
  At Thu, 10 May 2012 13:42:09 +0200,
  Takashi Iwai wrote:
 
  At Thu, 10 May 2012 12:20:05 +0100,
  Alan Cox wrote:
  
   On Thu, 10 May 2012 09:10:16 +0200
   Takashi Iwai ti...@suse.de wrote:
  
Add the support for audio clients to VGA-switcheroo for handling the
HDMI audio controller together with VGA switching.  The id of the
audio controller should be given explicity at registration time unlike
the video controller.
  
   It would I think be a lot cleaner and more future proof to pass a
  
   struct hdmi_audio_switch_ops
  
   or some similar named object with an array of function pointers ?
 
  That would be a good option, indeed.
 
  In my patch series, I just didn't want to break the existing API,
  so I kept the current style.
 
  Dave, do you prefer the way with an ops struct as Alan suggested?
 
  For example, I can make like
 
  struct vga_switcheroo_client_ops {
        void (*set_gpu_state)(struct pci_dev *dev, enum vga_switcheroo_state),
        void (*reprobe)(struct pci_dev *dev),
        bool (*can_switch)(struct pci_dev *dev));
  };
 
 Yeah I suppose we should go and just do that now, it kinda grew a bit
 much previously.
 
 If you want to do patches for it I'd be happy to take them.

OK, I'll work on it tomorrow.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

How to hook HDMI audio to vga-switcher?

[Takashi Iwai]

Hi Dave,

in kernel bug 43155, we found out that the HDMI audio controller on
AMD graphics is also disabled when the graphics chip is disabled via
vga-switcher:
https://bugzilla.kernel.org/show_bug.cgi?id=43155

This screws up the sound driver, takes too long time for boot.

Since the audio controller/codec becomes active again once when the
graphics is turned on, the best solution would be to add a client to
vga-swicheroo for handling the status of the HDMI audio controller.

But, looking at the current vga_switcheroo.c, it seems that the
current code assumes only two clients and doesn't consider about
non-graphics PCI entries.

As a workaround, I'm thiking of a sort of slave client belonging to
the video client, so that a callback like set_gpu_state() can be
called.

Or, if you have any better idea, let me know.


thanks,

Takashi

How to hook HDMI audio to vga-switcher?

[Takashi Iwai]

Hi Dave,

in kernel bug 43155, we found out that the HDMI audio controller on
AMD graphics is also disabled when the graphics chip is disabled via
vga-switcher:
https://bugzilla.kernel.org/show_bug.cgi?id=43155

This screws up the sound driver, takes too long time for boot.

Since the audio controller/codec becomes active again once when the
graphics is turned on, the best solution would be to add a client to
vga-swicheroo for handling the status of the HDMI audio controller.

But, looking at the current vga_switcheroo.c, it seems that the
current code assumes only two clients and doesn't consider about
non-graphics PCI entries.

As a workaround, I'm thiking of a sort of slave client belonging to
the video client, so that a callback like set_gpu_state() can be
called.

Or, if you have any better idea, let me know.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 20 Apr 2012 13:05:48 +0100,
Dave Airlie wrote:
> 
> On Thu, Apr 19, 2012 at 3:58 PM, Takashi Iwai  wrote:
> > At Thu, 19 Apr 2012 14:03:58 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Tue, 17 Apr 2012 17:26:26 +0200,
> >> Takashi Iwai wrote:
> >> >
> >> > At Fri, 13 Apr 2012 16:56:26 -0400,
> >> > Adam Jackson wrote:
> >> > >
> >> > > On 4/13/12 4:33 PM, Adam Jackson wrote:
> >> > > > Incorporates some feedback from Rodrigo and Takashi. ?Major themes 
> >> > > > of the
> >> > > > series:
> >> > > >
> >> > > > - Fix the DMT list to include reduced-blanking modes
> >> > > > - Add modes from DMT for any range descriptor type
> >> > > > - Add an extra modes list for things not in DMT
> >> > > > - For ranges that specify a formula, generate timings from the extra 
> >> > > > modes
> >> > > >
> >> > > > This still doesn't address the driver policy decision of "I know I 
> >> > > > have
> >> > > > a scaler, add modes as if there were a range descriptor even if 
> >> > > > there's
> >> > > > not one". ?But it should at least make clear what such a helper 
> >> > > > function
> >> > > > should do.
> >> > >
> >> > > One minor buglet in this series that's not obvious from inspection (and
> >> > > that I didn't realize until just now) is that putting 1366x768 in the
> >> > > minimode list won't do what you want, since the mode you get back will
> >> > > be 1368x768. ?Probably we should move the manual-underscan hack into 
> >> > > the
> >> > > timing generators themselves.
> >> >
> >> > Sounds like a good compromise. ?We have already hacks in drm_edid.c
> >> > for HDMI TV, so one exception more isn't that bad ;)
> 
> I've pulled the series into -next along with the NULL check fix.

Thanks!

> the only outstanding bit is the hack.

Also the packed attributes are missing in the new structs.
The fix patch is below.


Takashi

---

From: Takashi Iwai <ti...@suse.de>
Subject: [PATCH 2/2] drm/edid: Add packed attribute to new gtf2 and cvt structs

The new structs added in struct detailed_data_monitor_range must be
marked with packed attribute although the outer struct itself is
already marked as packed.  Otherwise these 7-bytes structs may be
aligned, and give the wrong position and size for the data.

Signed-off-by: Takashi Iwai 
---
 include/drm/drm_edid.h |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8cefbbe..0cac551 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -99,7 +99,7 @@ struct detailed_data_monitor_range {
__le16 m;
u8 k;
u8 j; /* need to divide by 2 */
-   } gtf2;
+   } __attribute__((packed)) gtf2;
struct {
u8 version;
u8 data1; /* high 6 bits: extra clock resolution */
@@ -108,7 +108,7 @@ struct detailed_data_monitor_range {
u8 flags; /* preferred aspect and blanking support */
u8 supported_scalings;
u8 preferred_refresh;
-   } cvt;
+   } __attribute__((packed)) cvt;
} formula;
 } __attribute__((packed));

-- 
1.7.9.2

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 20 Apr 2012 13:04:42 +0100,
Dave Airlie wrote:
> 
> On Thu, Apr 19, 2012 at 1:03 PM, Takashi Iwai  wrote:
> > At Tue, 17 Apr 2012 17:26:26 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Fri, 13 Apr 2012 16:56:26 -0400,
> >> Adam Jackson wrote:
> >> >
> >> > On 4/13/12 4:33 PM, Adam Jackson wrote:
> >> > > Incorporates some feedback from Rodrigo and Takashi. ?Major themes of 
> >> > > the
> >> > > series:
> >> > >
> >> > > - Fix the DMT list to include reduced-blanking modes
> >> > > - Add modes from DMT for any range descriptor type
> >> > > - Add an extra modes list for things not in DMT
> >> > > - For ranges that specify a formula, generate timings from the extra 
> >> > > modes
> >> > >
> >> > > This still doesn't address the driver policy decision of "I know I have
> >> > > a scaler, add modes as if there were a range descriptor even if there's
> >> > > not one". ?But it should at least make clear what such a helper 
> >> > > function
> >> > > should do.
> >> >
> >> > One minor buglet in this series that's not obvious from inspection (and
> >> > that I didn't realize until just now) is that putting 1366x768 in the
> >> > minimode list won't do what you want, since the mode you get back will
> >> > be 1368x768. ?Probably we should move the manual-underscan hack into the
> >> > timing generators themselves.
> >>
> >> Sounds like a good compromise. ?We have already hacks in drm_edid.c
> >> for HDMI TV, so one exception more isn't that bad ;)
> >
> > FYI, I tried the hack below and it seems working.
> >
> Is this hack going to be a real patch? ajax care to review?

For the easiness, below is the patch with my sign-off.


thanks,

Takashi

---

From: Takashi Iwai <ti...@suse.de>
Subject: [PATCH 1/2] drm/edid: Add a workaround for 1366x768 HD panel

HD panel (1366x768) found most commonly on laptops can't be represented
exactly in CVT/DMT expression, which leads to 1368x768 instead, because
1366 can't be divided by 8.

Add a hack to convert to 1366x768 manually as an exception.

Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/drm_edid.c |   15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bad2f11..c6366e9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1039,6 +1039,19 @@ drm_dmt_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
return modes;
 }

+/* fix up 1366x768 mode from 1368x768;
+ * GFT/CVT can't express 1366 width which isn't dividable by 8
+ */
+static void fixup_mode_1366x768(struct drm_display_mode *mode)
+{
+   if (mode->hdisplay == 1368 && mode->vdisplay == 768) {
+   mode->hdisplay = 1366;
+   mode->hsync_start--;
+   mode->hsync_end--;
+   drm_mode_set_name(mode);
+   }
+}
+
 static int
 drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid,
struct detailed_timing *timing)
@@ -1053,6 +1066,7 @@ drm_gtf_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
if (!newmode)
return modes;

+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
@@ -1080,6 +1094,7 @@ drm_cvt_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
if (!newmode)
return modes;

+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
-- 
1.7.9.2

Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 20 Apr 2012 13:04:42 +0100,
Dave Airlie wrote:
 
 On Thu, Apr 19, 2012 at 1:03 PM, Takashi Iwai ti...@suse.de wrote:
  At Tue, 17 Apr 2012 17:26:26 +0200,
  Takashi Iwai wrote:
 
  At Fri, 13 Apr 2012 16:56:26 -0400,
  Adam Jackson wrote:
  
   On 4/13/12 4:33 PM, Adam Jackson wrote:
Incorporates some feedback from Rodrigo and Takashi.  Major themes of 
the
series:
   
- Fix the DMT list to include reduced-blanking modes
- Add modes from DMT for any range descriptor type
- Add an extra modes list for things not in DMT
- For ranges that specify a formula, generate timings from the extra 
modes
   
This still doesn't address the driver policy decision of I know I have
a scaler, add modes as if there were a range descriptor even if there's
not one.  But it should at least make clear what such a helper 
function
should do.
  
   One minor buglet in this series that's not obvious from inspection (and
   that I didn't realize until just now) is that putting 1366x768 in the
   minimode list won't do what you want, since the mode you get back will
   be 1368x768.  Probably we should move the manual-underscan hack into the
   timing generators themselves.
 
  Sounds like a good compromise.  We have already hacks in drm_edid.c
  for HDMI TV, so one exception more isn't that bad ;)
 
  FYI, I tried the hack below and it seems working.
 
 Is this hack going to be a real patch? ajax care to review?

For the easiness, below is the patch with my sign-off.


thanks,

Takashi

---

From: Takashi Iwai ti...@suse.de
Subject: [PATCH 1/2] drm/edid: Add a workaround for 1366x768 HD panel

HD panel (1366x768) found most commonly on laptops can't be represented
exactly in CVT/DMT expression, which leads to 1368x768 instead, because
1366 can't be divided by 8.

Add a hack to convert to 1366x768 manually as an exception.

Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/drm/drm_edid.c |   15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bad2f11..c6366e9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1039,6 +1039,19 @@ drm_dmt_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
return modes;
 }
 
+/* fix up 1366x768 mode from 1368x768;
+ * GFT/CVT can't express 1366 width which isn't dividable by 8
+ */
+static void fixup_mode_1366x768(struct drm_display_mode *mode)
+{
+   if (mode-hdisplay == 1368  mode-vdisplay == 768) {
+   mode-hdisplay = 1366;
+   mode-hsync_start--;
+   mode-hsync_end--;
+   drm_mode_set_name(mode);
+   }
+}
+
 static int
 drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid,
struct detailed_timing *timing)
@@ -1053,6 +1066,7 @@ drm_gtf_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
if (!newmode)
return modes;
 
+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
@@ -1080,6 +1094,7 @@ drm_cvt_modes_for_range(struct drm_connector *connector, 
struct edid *edid,
if (!newmode)
return modes;
 
+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
-- 
1.7.9.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 20 Apr 2012 13:05:48 +0100,
Dave Airlie wrote:
 
 On Thu, Apr 19, 2012 at 3:58 PM, Takashi Iwai ti...@suse.de wrote:
  At Thu, 19 Apr 2012 14:03:58 +0200,
  Takashi Iwai wrote:
 
  At Tue, 17 Apr 2012 17:26:26 +0200,
  Takashi Iwai wrote:
  
   At Fri, 13 Apr 2012 16:56:26 -0400,
   Adam Jackson wrote:
   
On 4/13/12 4:33 PM, Adam Jackson wrote:
 Incorporates some feedback from Rodrigo and Takashi.  Major themes 
 of the
 series:

 - Fix the DMT list to include reduced-blanking modes
 - Add modes from DMT for any range descriptor type
 - Add an extra modes list for things not in DMT
 - For ranges that specify a formula, generate timings from the extra 
 modes

 This still doesn't address the driver policy decision of I know I 
 have
 a scaler, add modes as if there were a range descriptor even if 
 there's
 not one.  But it should at least make clear what such a helper 
 function
 should do.
   
One minor buglet in this series that's not obvious from inspection (and
that I didn't realize until just now) is that putting 1366x768 in the
minimode list won't do what you want, since the mode you get back will
be 1368x768.  Probably we should move the manual-underscan hack into 
the
timing generators themselves.
  
   Sounds like a good compromise.  We have already hacks in drm_edid.c
   for HDMI TV, so one exception more isn't that bad ;)
 
 I've pulled the series into -next along with the NULL check fix.

Thanks!

 the only outstanding bit is the hack.

Also the packed attributes are missing in the new structs.
The fix patch is below.


Takashi

---

From: Takashi Iwai ti...@suse.de
Subject: [PATCH 2/2] drm/edid: Add packed attribute to new gtf2 and cvt structs

The new structs added in struct detailed_data_monitor_range must be
marked with packed attribute although the outer struct itself is
already marked as packed.  Otherwise these 7-bytes structs may be
aligned, and give the wrong position and size for the data.

Signed-off-by: Takashi Iwai ti...@suse.de
---
 include/drm/drm_edid.h |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8cefbbe..0cac551 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -99,7 +99,7 @@ struct detailed_data_monitor_range {
__le16 m;
u8 k;
u8 j; /* need to divide by 2 */
-   } gtf2;
+   } __attribute__((packed)) gtf2;
struct {
u8 version;
u8 data1; /* high 6 bits: extra clock resolution */
@@ -108,7 +108,7 @@ struct detailed_data_monitor_range {
u8 flags; /* preferred aspect and blanking support */
u8 supported_scalings;
u8 preferred_refresh;
-   } cvt;
+   } __attribute__((packed)) cvt;
} formula;
 } __attribute__((packed));
 
-- 
1.7.9.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Thu, 19 Apr 2012 14:03:58 +0200,
Takashi Iwai wrote:
> 
> At Tue, 17 Apr 2012 17:26:26 +0200,
> Takashi Iwai wrote:
> > 
> > At Fri, 13 Apr 2012 16:56:26 -0400,
> > Adam Jackson wrote:
> > > 
> > > On 4/13/12 4:33 PM, Adam Jackson wrote:
> > > > Incorporates some feedback from Rodrigo and Takashi.  Major themes of 
> > > > the
> > > > series:
> > > >
> > > > - Fix the DMT list to include reduced-blanking modes
> > > > - Add modes from DMT for any range descriptor type
> > > > - Add an extra modes list for things not in DMT
> > > > - For ranges that specify a formula, generate timings from the extra 
> > > > modes
> > > >
> > > > This still doesn't address the driver policy decision of "I know I have
> > > > a scaler, add modes as if there were a range descriptor even if there's
> > > > not one".  But it should at least make clear what such a helper function
> > > > should do.
> > > 
> > > One minor buglet in this series that's not obvious from inspection (and 
> > > that I didn't realize until just now) is that putting 1366x768 in the 
> > > minimode list won't do what you want, since the mode you get back will 
> > > be 1368x768.  Probably we should move the manual-underscan hack into the 
> > > timing generators themselves.
> > 
> > Sounds like a good compromise.  We have already hacks in drm_edid.c
> > for HDMI TV, so one exception more isn't that bad ;)
> 
> FYI, I tried the hack below and it seems working.

In addition, there are some missing NULL checks...


Takashi

---
---
 drivers/gpu/drm/drm_edid.c |6 ++
 1 file changed, 6 insertions(+)

--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -789,6 +789,8 @@ drm_mode_std(struct drm_connector *conne
 * secondary GTF curve.  Please don't do that.
 */
mode = drm_gtf_mode(dev, hsize, vsize, vrefresh_rate, 0, 0);
+   if (!mode)
+   return NULL;
if (drm_mode_hsync(mode) > drm_gtf2_hbreak(edid)) {
kfree(mode);
mode = drm_gtf_mode_complex(dev, hsize, vsize,
@@ -1071,6 +1073,8 @@ drm_gtf_modes_for_range(struct drm_conne
for (i = 0; i < num_extra_modes; i++) {
const struct minimode *m = _modes[i];
newmode = drm_gtf_mode(dev, m->w, m->h, m->r, 0, 0);
+   if (!newmode)
+   return modes;

fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
@@ -1097,6 +1101,8 @@ drm_cvt_modes_for_range(struct drm_conne
for (i = 0; i < num_extra_modes; i++) {
const struct minimode *m = _modes[i];
newmode = drm_cvt_mode(dev, m->w, m->h, m->r, rb, 0, 0);
+   if (!newmode)
+   return modes;

fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Tue, 17 Apr 2012 17:26:26 +0200,
Takashi Iwai wrote:
> 
> At Fri, 13 Apr 2012 16:56:26 -0400,
> Adam Jackson wrote:
> > 
> > On 4/13/12 4:33 PM, Adam Jackson wrote:
> > > Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
> > > series:
> > >
> > > - Fix the DMT list to include reduced-blanking modes
> > > - Add modes from DMT for any range descriptor type
> > > - Add an extra modes list for things not in DMT
> > > - For ranges that specify a formula, generate timings from the extra modes
> > >
> > > This still doesn't address the driver policy decision of "I know I have
> > > a scaler, add modes as if there were a range descriptor even if there's
> > > not one".  But it should at least make clear what such a helper function
> > > should do.
> > 
> > One minor buglet in this series that's not obvious from inspection (and 
> > that I didn't realize until just now) is that putting 1366x768 in the 
> > minimode list won't do what you want, since the mode you get back will 
> > be 1368x768.  Probably we should move the manual-underscan hack into the 
> > timing generators themselves.
> 
> Sounds like a good compromise.  We have already hacks in drm_edid.c
> for HDMI TV, so one exception more isn't that bad ;)

FYI, I tried the hack below and it seems working.


Takashi

---
---
 drivers/gpu/drm/drm_edid.c |   15 +++
 1 file changed, 15 insertions(+)

--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1047,6 +1047,19 @@ drm_dmt_modes_for_range(struct drm_conne
return modes;
 }

+/* fix up 1366x768 mode from 1368x768;
+ * GTF/CVT can't express 1366 width which isn't dividable by 8
+ */
+static void fixup_mode_1366x768(struct drm_display_mode *mode)
+{
+   if (mode->hdisplay == 1368 && mode->vdisplay == 768) {
+   mode->hdisplay = 1366;
+   mode->hsync_start--;
+   mode->hsync_end--;
+   drm_mode_set_name(mode);
+   }
+}
+
 static int
 drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid,
struct detailed_timing *timing)
@@ -1059,6 +1072,7 @@ drm_gtf_modes_for_range(struct drm_conne
const struct minimode *m = _modes[i];
newmode = drm_gtf_mode(dev, m->w, m->h, m->r, 0, 0);

+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
@@ -1084,6 +1098,7 @@ drm_cvt_modes_for_range(struct drm_conne
const struct minimode *m = _modes[i];
newmode = drm_cvt_mode(dev, m->w, m->h, m->r, rb, 0, 0);

+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;

Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Tue, 17 Apr 2012 17:26:26 +0200,
Takashi Iwai wrote:
 
 At Fri, 13 Apr 2012 16:56:26 -0400,
 Adam Jackson wrote:
  
  On 4/13/12 4:33 PM, Adam Jackson wrote:
   Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
   series:
  
   - Fix the DMT list to include reduced-blanking modes
   - Add modes from DMT for any range descriptor type
   - Add an extra modes list for things not in DMT
   - For ranges that specify a formula, generate timings from the extra modes
  
   This still doesn't address the driver policy decision of I know I have
   a scaler, add modes as if there were a range descriptor even if there's
   not one.  But it should at least make clear what such a helper function
   should do.
  
  One minor buglet in this series that's not obvious from inspection (and 
  that I didn't realize until just now) is that putting 1366x768 in the 
  minimode list won't do what you want, since the mode you get back will 
  be 1368x768.  Probably we should move the manual-underscan hack into the 
  timing generators themselves.
 
 Sounds like a good compromise.  We have already hacks in drm_edid.c
 for HDMI TV, so one exception more isn't that bad ;)

FYI, I tried the hack below and it seems working.


Takashi

---
---
 drivers/gpu/drm/drm_edid.c |   15 +++
 1 file changed, 15 insertions(+)

--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1047,6 +1047,19 @@ drm_dmt_modes_for_range(struct drm_conne
return modes;
 }
 
+/* fix up 1366x768 mode from 1368x768;
+ * GTF/CVT can't express 1366 width which isn't dividable by 8
+ */
+static void fixup_mode_1366x768(struct drm_display_mode *mode)
+{
+   if (mode-hdisplay == 1368  mode-vdisplay == 768) {
+   mode-hdisplay = 1366;
+   mode-hsync_start--;
+   mode-hsync_end--;
+   drm_mode_set_name(mode);
+   }
+}
+
 static int
 drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid,
struct detailed_timing *timing)
@@ -1059,6 +1072,7 @@ drm_gtf_modes_for_range(struct drm_conne
const struct minimode *m = extra_modes[i];
newmode = drm_gtf_mode(dev, m-w, m-h, m-r, 0, 0);
 
+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
@@ -1084,6 +1098,7 @@ drm_cvt_modes_for_range(struct drm_conne
const struct minimode *m = extra_modes[i];
newmode = drm_cvt_mode(dev, m-w, m-h, m-r, rb, 0, 0);
 
+   fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
drm_mode_destroy(dev, newmode);
continue;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Thu, 19 Apr 2012 14:03:58 +0200,
Takashi Iwai wrote:
 
 At Tue, 17 Apr 2012 17:26:26 +0200,
 Takashi Iwai wrote:
  
  At Fri, 13 Apr 2012 16:56:26 -0400,
  Adam Jackson wrote:
   
   On 4/13/12 4:33 PM, Adam Jackson wrote:
Incorporates some feedback from Rodrigo and Takashi.  Major themes of 
the
series:
   
- Fix the DMT list to include reduced-blanking modes
- Add modes from DMT for any range descriptor type
- Add an extra modes list for things not in DMT
- For ranges that specify a formula, generate timings from the extra 
modes
   
This still doesn't address the driver policy decision of I know I have
a scaler, add modes as if there were a range descriptor even if there's
not one.  But it should at least make clear what such a helper function
should do.
   
   One minor buglet in this series that's not obvious from inspection (and 
   that I didn't realize until just now) is that putting 1366x768 in the 
   minimode list won't do what you want, since the mode you get back will 
   be 1368x768.  Probably we should move the manual-underscan hack into the 
   timing generators themselves.
  
  Sounds like a good compromise.  We have already hacks in drm_edid.c
  for HDMI TV, so one exception more isn't that bad ;)
 
 FYI, I tried the hack below and it seems working.

In addition, there are some missing NULL checks...


Takashi

---
---
 drivers/gpu/drm/drm_edid.c |6 ++
 1 file changed, 6 insertions(+)

--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -789,6 +789,8 @@ drm_mode_std(struct drm_connector *conne
 * secondary GTF curve.  Please don't do that.
 */
mode = drm_gtf_mode(dev, hsize, vsize, vrefresh_rate, 0, 0);
+   if (!mode)
+   return NULL;
if (drm_mode_hsync(mode)  drm_gtf2_hbreak(edid)) {
kfree(mode);
mode = drm_gtf_mode_complex(dev, hsize, vsize,
@@ -1071,6 +1073,8 @@ drm_gtf_modes_for_range(struct drm_conne
for (i = 0; i  num_extra_modes; i++) {
const struct minimode *m = extra_modes[i];
newmode = drm_gtf_mode(dev, m-w, m-h, m-r, 0, 0);
+   if (!newmode)
+   return modes;
 
fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
@@ -1097,6 +1101,8 @@ drm_cvt_modes_for_range(struct drm_conne
for (i = 0; i  num_extra_modes; i++) {
const struct minimode *m = extra_modes[i];
newmode = drm_cvt_mode(dev, m-w, m-h, m-r, rb, 0, 0);
+   if (!newmode)
+   return modes;
 
fixup_mode_1366x768(newmode);
if (!mode_in_range(newmode, edid, timing)) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/radeon/kms: fix the regression of DVI connector check

[Takashi Iwai]

The check of the encoder type in the commit [e00e8b5e: drm/radeon/kms:
fix analog load detection on DVI-I connectors] is obviously wrong, and
it's the culprit of the regression on my workstation with DVI-analog
connection resulting in the blank output.

Fixed the typo now.

Cc: 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c 
b/drivers/gpu/drm/radeon/radeon_connectors.c
index bd05156..aa8268d 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -970,7 +970,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool 
force)

encoder = obj_to_encoder(obj);

-   if (encoder->encoder_type != DRM_MODE_ENCODER_DAC ||
+   if (encoder->encoder_type != DRM_MODE_ENCODER_DAC &&
encoder->encoder_type != DRM_MODE_ENCODER_TVDAC)
continue;

-- 
1.7.9.2

[REGRESSION] 3.4-rc* radeon: No DVI-A output after commit e00e8b5e760cb

[Takashi Iwai]

At Wed, 18 Apr 2012 14:39:54 +0200,
Takashi Iwai wrote:
> 
> Hi,
> 
> I noticed that one machine here gives only the blank output with
> 3.4-rc's.  The bisection lead me to affecting commit:
> 
> commit e00e8b5e760cbbe9067daeae5454d67c44c8d035
> Author: Alex Deucher 
> Date:   Fri Mar 16 12:22:09 2012 -0400
> 
> drm/radeon/kms: fix analog load detection on DVI-I connectors
> 
> Reverting this commit helped, it goes back to normal.
> 
> On this system, the VGA monitor is connected to DVI over a VGA-DVI
> connector (and even VGA-switcher).  So, yeah, it's a strange setup.
> But a regression is a regression.

Looking at the commit again, it appears like a simple logic failure.
It should be "&&" instead of "||".


Takashi

[REGRESSION] 3.4-rc* radeon: No DVI-A output after commit e00e8b5e760cb

[Takashi Iwai]

Hi,

I noticed that one machine here gives only the blank output with
3.4-rc's.  The bisection lead me to affecting commit:

commit e00e8b5e760cbbe9067daeae5454d67c44c8d035
Author: Alex Deucher 
Date:   Fri Mar 16 12:22:09 2012 -0400

drm/radeon/kms: fix analog load detection on DVI-I connectors

Reverting this commit helped, it goes back to normal.

On this system, the VGA monitor is connected to DVI over a VGA-DVI
connector (and even VGA-switcher).  So, yeah, it's a strange setup.
But a regression is a regression.

I'm willing to test any patch.


thanks,

Takashi

[REGRESSION] 3.4-rc* radeon: No DVI-A output after commit e00e8b5e760cb

[Takashi Iwai]

Hi,

I noticed that one machine here gives only the blank output with
3.4-rc's.  The bisection lead me to affecting commit:

commit e00e8b5e760cbbe9067daeae5454d67c44c8d035
Author: Alex Deucher alexander.deuc...@amd.com
Date:   Fri Mar 16 12:22:09 2012 -0400

drm/radeon/kms: fix analog load detection on DVI-I connectors

Reverting this commit helped, it goes back to normal.

On this system, the VGA monitor is connected to DVI over a VGA-DVI
connector (and even VGA-switcher).  So, yeah, it's a strange setup.
But a regression is a regression.

I'm willing to test any patch.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [REGRESSION] 3.4-rc* radeon: No DVI-A output after commit e00e8b5e760cb

[Takashi Iwai]

At Wed, 18 Apr 2012 14:39:54 +0200,
Takashi Iwai wrote:
 
 Hi,
 
 I noticed that one machine here gives only the blank output with
 3.4-rc's.  The bisection lead me to affecting commit:
 
 commit e00e8b5e760cbbe9067daeae5454d67c44c8d035
 Author: Alex Deucher alexander.deuc...@amd.com
 Date:   Fri Mar 16 12:22:09 2012 -0400
 
 drm/radeon/kms: fix analog load detection on DVI-I connectors
 
 Reverting this commit helped, it goes back to normal.
 
 On this system, the VGA monitor is connected to DVI over a VGA-DVI
 connector (and even VGA-switcher).  So, yeah, it's a strange setup.
 But a regression is a regression.

Looking at the commit again, it appears like a simple logic failure.
It should be  instead of ||.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/radeon/kms: fix the regression of DVI connector check

[Takashi Iwai]

The check of the encoder type in the commit [e00e8b5e: drm/radeon/kms:
fix analog load detection on DVI-I connectors] is obviously wrong, and
it's the culprit of the regression on my workstation with DVI-analog
connection resulting in the blank output.

Fixed the typo now.

Cc: sta...@vger.kernel.org
Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c 
b/drivers/gpu/drm/radeon/radeon_connectors.c
index bd05156..aa8268d 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -970,7 +970,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool 
force)
 
encoder = obj_to_encoder(obj);
 
-   if (encoder-encoder_type != DRM_MODE_ENCODER_DAC ||
+   if (encoder-encoder_type != DRM_MODE_ENCODER_DAC 
encoder-encoder_type != DRM_MODE_ENCODER_TVDAC)
continue;
 
-- 
1.7.9.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Tue, 17 Apr 2012 17:33:17 +0200,
Takashi Iwai wrote:
> 
> At Fri, 13 Apr 2012 16:33:28 -0400,
> Adam Jackson wrote:
> > 
> > Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
> > series:
> > 
> > - Fix the DMT list to include reduced-blanking modes
> > - Add modes from DMT for any range descriptor type
> > - Add an extra modes list for things not in DMT
> > - For ranges that specify a formula, generate timings from the extra modes
> > 
> > This still doesn't address the driver policy decision of "I know I have
> > a scaler, add modes as if there were a range descriptor even if there's
> > not one".  But it should at least make clear what such a helper function
> > should do.
> 
> I tried these patches now with a few monitors here and it looks
> working well (after fixing the alignment as I posted in another
> mail).  I got new 1600x900 output on two monitors.  Yay!

Oh, feel free to add my tested-by tag:

Tested-by: Takashi Iwai 


thanks,

Takashi

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 13 Apr 2012 16:33:28 -0400,
Adam Jackson wrote:
> 
> Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
> series:
> 
> - Fix the DMT list to include reduced-blanking modes
> - Add modes from DMT for any range descriptor type
> - Add an extra modes list for things not in DMT
> - For ranges that specify a formula, generate timings from the extra modes
> 
> This still doesn't address the driver policy decision of "I know I have
> a scaler, add modes as if there were a range descriptor even if there's
> not one".  But it should at least make clear what such a helper function
> should do.

I tried these patches now with a few monitors here and it looks
working well (after fixing the alignment as I posted in another
mail).  I got new 1600x900 output on two monitors.  Yay!

I guess this will solve major of problems we've face for HD+ panel.
The rest is the 1366x768 mode, but it's often covered by 1360x768.
So, once when LVDS gets additional de facto standard modes, it'll be
fixed as well.


thanks,

Takashi

[Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Fri, 13 Apr 2012 16:56:26 -0400,
Adam Jackson wrote:
> 
> On 4/13/12 4:33 PM, Adam Jackson wrote:
> > Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
> > series:
> >
> > - Fix the DMT list to include reduced-blanking modes
> > - Add modes from DMT for any range descriptor type
> > - Add an extra modes list for things not in DMT
> > - For ranges that specify a formula, generate timings from the extra modes
> >
> > This still doesn't address the driver policy decision of "I know I have
> > a scaler, add modes as if there were a range descriptor even if there's
> > not one".  But it should at least make clear what such a helper function
> > should do.
> 
> One minor buglet in this series that's not obvious from inspection (and 
> that I didn't realize until just now) is that putting 1366x768 in the 
> minimode list won't do what you want, since the mode you get back will 
> be 1368x768.  Probably we should move the manual-underscan hack into the 
> timing generators themselves.

Sounds like a good compromise.  We have already hacks in drm_edid.c
for HDMI TV, so one exception more isn't that bad ;)


Takashi

[Intel-gfx] [PATCH 09/12] drm/edid: Update range descriptor struct for EDID 1.4

[Takashi Iwai]

At Fri, 13 Apr 2012 16:33:37 -0400,
Adam Jackson wrote:
> 
> Signed-off-by: Adam Jackson 
> ---
>  include/drm/drm_edid.h |   26 --
>  1 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index bcb9a66..8cefbbe 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -90,12 +90,26 @@ struct detailed_data_monitor_range {
>   u8 min_hfreq_khz;
>   u8 max_hfreq_khz;
>   u8 pixel_clock_mhz; /* need to multiply by 10 */
> - __le16 sec_gtf_toggle; /* A000=use above, 20=use below */
> - u8 hfreq_start_khz; /* need to multiply by 2 */
> - u8 c; /* need to divide by 2 */
> - __le16 m;
> - u8 k;
> - u8 j; /* need to divide by 2 */
> + u8 flags;
> + union {
> + struct {
> + u8 reserved;
> + u8 hfreq_start_khz; /* need to multiply by 2 */
> + u8 c; /* need to divide by 2 */
> + __le16 m;
> + u8 k;
> + u8 j; /* need to divide by 2 */
> + } gtf2;
> + struct {
> + u8 version;
> + u8 data1; /* high 6 bits: extra clock resolution */
> + u8 data2; /* plus low 2 of above: max hactive */
> + u8 supported_aspects;
> + u8 flags; /* preferred aspect and blanking support */
> + u8 supported_scalings;
> + u8 preferred_refresh;
> + } cvt;

These new structs must be marked with __attribute__((packed)) although
the struct detailed_data_monitor_range itself is already marked.  At
least, with gcc 4.6 and x86-64 here, they get unaligned.


thanks,

Takashi

> + } formula;
>  } __attribute__((packed));
>  
>  struct detailed_data_wpindex {
> -- 
> 1.7.7.6
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [Intel-gfx] [PATCH 09/12] drm/edid: Update range descriptor struct for EDID 1.4

[Takashi Iwai]

At Fri, 13 Apr 2012 16:33:37 -0400,
Adam Jackson wrote:
 
 Signed-off-by: Adam Jackson a...@redhat.com
 ---
  include/drm/drm_edid.h |   26 --
  1 files changed, 20 insertions(+), 6 deletions(-)
 
 diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
 index bcb9a66..8cefbbe 100644
 --- a/include/drm/drm_edid.h
 +++ b/include/drm/drm_edid.h
 @@ -90,12 +90,26 @@ struct detailed_data_monitor_range {
   u8 min_hfreq_khz;
   u8 max_hfreq_khz;
   u8 pixel_clock_mhz; /* need to multiply by 10 */
 - __le16 sec_gtf_toggle; /* A000=use above, 20=use below */
 - u8 hfreq_start_khz; /* need to multiply by 2 */
 - u8 c; /* need to divide by 2 */
 - __le16 m;
 - u8 k;
 - u8 j; /* need to divide by 2 */
 + u8 flags;
 + union {
 + struct {
 + u8 reserved;
 + u8 hfreq_start_khz; /* need to multiply by 2 */
 + u8 c; /* need to divide by 2 */
 + __le16 m;
 + u8 k;
 + u8 j; /* need to divide by 2 */
 + } gtf2;
 + struct {
 + u8 version;
 + u8 data1; /* high 6 bits: extra clock resolution */
 + u8 data2; /* plus low 2 of above: max hactive */
 + u8 supported_aspects;
 + u8 flags; /* preferred aspect and blanking support */
 + u8 supported_scalings;
 + u8 preferred_refresh;
 + } cvt;

These new structs must be marked with __attribute__((packed)) although
the struct detailed_data_monitor_range itself is already marked.  At
least, with gcc 4.6 and x86-64 here, they get unaligned.


thanks,

Takashi

 + } formula;
  } __attribute__((packed));
  
  struct detailed_data_wpindex {
 -- 
 1.7.7.6
 
 ___
 Intel-gfx mailing list
 intel-...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes

[Takashi Iwai]

At Tue, 17 Apr 2012 17:33:17 +0200,
Takashi Iwai wrote:
 
 At Fri, 13 Apr 2012 16:33:28 -0400,
 Adam Jackson wrote:
  
  Incorporates some feedback from Rodrigo and Takashi.  Major themes of the
  series:
  
  - Fix the DMT list to include reduced-blanking modes
  - Add modes from DMT for any range descriptor type
  - Add an extra modes list for things not in DMT
  - For ranges that specify a formula, generate timings from the extra modes
  
  This still doesn't address the driver policy decision of I know I have
  a scaler, add modes as if there were a range descriptor even if there's
  not one.  But it should at least make clear what such a helper function
  should do.
 
 I tried these patches now with a few monitors here and it looks
 working well (after fixing the alignment as I posted in another
 mail).  I got new 1600x900 output on two monitors.  Yay!

Oh, feel free to add my tested-by tag:

Tested-by: Takashi Iwai ti...@suse.de


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 12:31:25 -0400,
Adam Jackson wrote:
> 
> On 4/13/12 11:41 AM, Takashi Iwai wrote:
> > At Fri, 13 Apr 2012 11:30:01 -0400, Alex Deucher wrote:
> >> One thing to be careful of is that some monitors (especially LCD
> >> panels) don't like modes that are not in their EDIDs.  As such when
> >> you try and set them you often get a wonky display or more often a
> >> blank screen.  We used to add a lot of inferred modes to the mode list
> >> in the xserver which resulted in a lot of blank screens when some odd
> >> mode was picked as the best match for a cloned display.  The "fix" was
> >> to only add the inferred modes on analog monitors which were more
> >> likely to be able to support them.
> >
> > Thanks, it's good to know!
> >
> > Though, I still wonder whether adding inferred modes for 1366x768 or
> > 1600x900 would cause any big problems.  On such monitors, 1360x768 or
> > 1440x900 (or 1680x1050) are usually seen in the supported list.
> >
> > Of course, it's never 100% safe.  But not so bad odds?
> 
> "Mostly working" is a fancy way of saying "broken".

True.

> The semantics of the range descriptor are "I can support modes within 
> these ranges generated by these timing formulas and/or listed in DMT". 
> That's why we only walk that mode list when we find a range descriptor: 
> if you _don't_ find a range descriptor then the monitor is explicitly 
> telling you it doesn't support arbitrary modes over a range.
> 
> You can be more aggressive than that if you know your CRTC's scaler can 
> compensate, in which case you'd run the display at the native mode and 
> let the scaler translate.  The EDID parser is currently not told that 
> bit of context, and in a sense it really shouldn't; it would be a 
> function of the CRTC and the DMT list, independent of whether you have 
> EDID at all.
> 
> It's not especially hard to add, I suppose.  You'd want to mark modes so 
> added so the CRTC setup knows to do the appropriate panel magic.

Right, we need some way to verify that the mode is acceptable.
It's what Rodirgo wants to figure out now...


Takashi

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 11:30:01 -0400,
Alex Deucher wrote:
> 
> On Fri, Apr 13, 2012 at 11:13 AM, Takashi Iwai  wrote:
> > At Fri, 13 Apr 2012 15:35:04 +0100,
> > Dave Airlie wrote:
> >>
> >> > I don't think we need to support all wild modes, too. ?But the _very_
> >> > common modes like 1366x768 and 1600x900 should be really supported as
> >> > default.
> >>
> >> You guys still haven't answered the basic question, what HW is this broken?
> >
> > The reported problem is about HP laptops with i915 driver (no matter
> > chip chip is) and several monitors with resolutions more than the
> > laptop panel.
> >
> > The LVDS provides only the native resolution (either 1366x768 or
> > 1600x900) and a few other VESA ones (1024x768, 800x600 and 640x480).
> > Meanwhile, the monitor EDID doesn't provide such laptop-native
> > resolutions.
> > Thus, in clone mode, the only possible resolution is 1024x768 or
> > lower. ?That's the whole problem. ?It's too low and doesn't match with
> > 16:9 although both laptop and monitor panels are 16:9.
> >
> > HP wants the clone mode of the laptop-native resolution and/or a
> > higher resolution with the right aspect ratio like 1280x720. ?Neither
> > work as of now unless you add the extra mode manually.
> >
> 
> One thing to be careful of is that some monitors (especially LCD
> panels) don't like modes that are not in their EDIDs.  As such when
> you try and set them you often get a wonky display or more often a
> blank screen.  We used to add a lot of inferred modes to the mode list
> in the xserver which resulted in a lot of blank screens when some odd
> mode was picked as the best match for a cloned display.  The "fix" was
> to only add the inferred modes on analog monitors which were more
> likely to be able to support them.

Thanks, it's good to know!

Though, I still wonder whether adding inferred modes for 1366x768 or 
1600x900 would cause any big problems.  On such monitors, 1360x768 or
1440x900 (or 1680x1050) are usually seen in the supported list.

Of course, it's never 100% safe.  But not so bad odds?


Takashi

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 10:55:16 -0400,
Adam Jackson wrote:
> 
> On 4/13/12 10:29 AM, Takashi Iwai wrote:
> > At Fri, 13 Apr 2012 10:14:46 -0400,
> > Adam Jackson wrote:
> >> Yeah, that's a bug.  That's why I said it should be renamed
> >> drm_dmt_modes_for_range and run unconditionally if we find a range
> >> descriptor.
> >
> > Yeah, I saw your patches.  Should the further work base on them?
> 
> Would be nice.
> 
> > Yesterday I've read a news reporting that 1366x768 is the most
> > commonly used panel now, more than 1024x768.  And, 1600x900 is in the
> > second place of the modern laptop panels.
> >
> > Windows and others do work with these resolutions on the same
> > monitor.  Why Linux driver can't?  Everbody (but developers) thinks
> > like that way.
> 
> I think you're trying to make me defend a position I wasn't taking...

Heh, don't take it offensive.

> >> If it's not the native panel size and it's not a commonly found size in
> >> the wild, why are we obligated to provide them for every user?  Remember
> >> that userspace has the ability to hand in modes from above.
> >
> > I don't think we need to support all wild modes, too.  But the _very_
> > common modes like 1366x768 and 1600x900 should be really supported as
> > default.
> 
> I'm not disagreeing.  I think common sizes should be available, and we 
> have code already that's intended to do that.

OK.

> My issue with the list in the patch is it contains some nonsense.  If 
> some of those more weird-looking sizes _do_ exist in the wild they 
> should be already present in EDID as the native size.  For panels where 
> they're not native I have difficulty imagining anyone wanting to set 
> that mode intentionally.  And for someone who really does want it they 
> have the ability to pass in arbitrary crap from userspace anyway.
> 
> 1600x900 is reasonable to add to an "extras" list, because it is 
> actually common despite not being in DMT.  I'm even willing to take 
> Windows as the example for what modes should be in the extras list.  But 
> I'm not willing to take "I wish this was in a preset list" as the sole 
> justification.

I agree with your point, too.  When I worked on supporting these modes
in X server side, I didn't pick up all such modes but only really de
facto standard ones.  It should suffice for most demands, indeed.

Also, we don't have to add always 1600x900 or 1366x768.  Such a mode
is necessary basically only when the laptop panel resolution isn't
found in the mode list.  We may add it selectively, too.


thanks,

Takashi

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 15:35:04 +0100,
Dave Airlie wrote:
> 
> > I don't think we need to support all wild modes, too. ?But the _very_
> > common modes like 1366x768 and 1600x900 should be really supported as
> > default.
> 
> You guys still haven't answered the basic question, what HW is this broken?

The reported problem is about HP laptops with i915 driver (no matter
chip chip is) and several monitors with resolutions more than the
laptop panel.

The LVDS provides only the native resolution (either 1366x768 or
1600x900) and a few other VESA ones (1024x768, 800x600 and 640x480).
Meanwhile, the monitor EDID doesn't provide such laptop-native
resolutions.
Thus, in clone mode, the only possible resolution is 1024x768 or
lower.  That's the whole problem.  It's too low and doesn't match with
16:9 although both laptop and monitor panels are 16:9.

HP wants the clone mode of the laptop-native resolution and/or a
higher resolution with the right aspect ratio like 1280x720.  Neither
work as of now unless you add the extra mode manually.

> Can you provide some EDIDs?

OK, here we go.  Outputs of "xrandr --verbose" and EDID data from two
monitors.


thanks,

Takashi
-- next part --
A non-text attachment was scrubbed...
Name: xrandr1
Type: application/octet-stream
Size: 10715 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: xrandr2
Type: application/octet-stream
Size: 14961 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: edid1
Type: application/octet-stream
Size: 128 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: edid2
Type: application/octet-stream
Size: 128 bytes
Desc: not available
URL: 

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 10:14:46 -0400,
Adam Jackson wrote:
> 
> On 4/12/12 7:09 PM, Rodrigo Vivi wrote:
> 
> >> CVT monitors _must_ accept GTF as well, EDID says so.  So functionally
> >> there's nothing wrong with the existing code.
> >
> > Actually the current code just check for gtf flag... so if a monitor
> > is gtf2 or cvt this dmt modes are not being added.
> 
> Yeah, that's a bug.  That's why I said it should be renamed 
> drm_dmt_modes_for_range and run unconditionally if we find a range 
> descriptor.

Yeah, I saw your patches.  Should the further work base on them?

> >> The thing you're trying
> >> to sneak in here is a 1600x900 timing that doesn't correspond to
> >> anything in DMT (at least, not in the copy of DMT that I have handy).
> >> It's fine to want to add more modes - although I'm still unclear exactly
> >> which machine you're trying to compensate for here - but not if it comes
> >> by sacrificing the DMT list, which is there for a reason.
> >
> > There are customers complaining about lots of missing modes that are
> > supported by windows and/or other drivers and we are missing. If this
> > modes are ok on edid spec I se no problem in add them... ok.. I don't
> > like hardcoded as well... I think we could find another way to invent
> > this modes and use the cvt function to calculate timings... or
> > something like that.
> 
> Why are they complaining, and why do you think you're obligated to care?

Because it's his job.  And mine, too.  What a pity.

Yesterday I've read a news reporting that 1366x768 is the most
commonly used panel now, more than 1024x768.  And, 1600x900 is in the
second place of the modern laptop panels.

Windows and others do work with these resolutions on the same
monitor.  Why Linux driver can't?  Everbody (but developers) thinks
like that way.

> If it's not the native panel size and it's not a commonly found size in 
> the wild, why are we obligated to provide them for every user?  Remember 
> that userspace has the ability to hand in modes from above.

I don't think we need to support all wild modes, too.  But the _very_
common modes like 1366x768 and 1600x900 should be really supported as
default.


thanks,

Takashi

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 10:14:46 -0400,
Adam Jackson wrote:
 
 On 4/12/12 7:09 PM, Rodrigo Vivi wrote:
 
  CVT monitors _must_ accept GTF as well, EDID says so.  So functionally
  there's nothing wrong with the existing code.
 
  Actually the current code just check for gtf flag... so if a monitor
  is gtf2 or cvt this dmt modes are not being added.
 
 Yeah, that's a bug.  That's why I said it should be renamed 
 drm_dmt_modes_for_range and run unconditionally if we find a range 
 descriptor.

Yeah, I saw your patches.  Should the further work base on them?

  The thing you're trying
  to sneak in here is a 1600x900 timing that doesn't correspond to
  anything in DMT (at least, not in the copy of DMT that I have handy).
  It's fine to want to add more modes - although I'm still unclear exactly
  which machine you're trying to compensate for here - but not if it comes
  by sacrificing the DMT list, which is there for a reason.
 
  There are customers complaining about lots of missing modes that are
  supported by windows and/or other drivers and we are missing. If this
  modes are ok on edid spec I se no problem in add them... ok.. I don't
  like hardcoded as well... I think we could find another way to invent
  this modes and use the cvt function to calculate timings... or
  something like that.
 
 Why are they complaining, and why do you think you're obligated to care?

Because it's his job.  And mine, too.  What a pity.

Yesterday I've read a news reporting that 1366x768 is the most
commonly used panel now, more than 1024x768.  And, 1600x900 is in the
second place of the modern laptop panels.

Windows and others do work with these resolutions on the same
monitor.  Why Linux driver can't?  Everbody (but developers) thinks
like that way.

 If it's not the native panel size and it's not a commonly found size in 
 the wild, why are we obligated to provide them for every user?  Remember 
 that userspace has the ability to hand in modes from above.

I don't think we need to support all wild modes, too.  But the _very_
common modes like 1366x768 and 1600x900 should be really supported as
default.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 15:35:04 +0100,
Dave Airlie wrote:
 
  I don't think we need to support all wild modes, too.  But the _very_
  common modes like 1366x768 and 1600x900 should be really supported as
  default.
 
 You guys still haven't answered the basic question, what HW is this broken?

The reported problem is about HP laptops with i915 driver (no matter
chip chip is) and several monitors with resolutions more than the
laptop panel.

The LVDS provides only the native resolution (either 1366x768 or
1600x900) and a few other VESA ones (1024x768, 800x600 and 640x480).
Meanwhile, the monitor EDID doesn't provide such laptop-native
resolutions.
Thus, in clone mode, the only possible resolution is 1024x768 or
lower.  That's the whole problem.  It's too low and doesn't match with
16:9 although both laptop and monitor panels are 16:9.

HP wants the clone mode of the laptop-native resolution and/or a
higher resolution with the right aspect ratio like 1280x720.  Neither
work as of now unless you add the extra mode manually.

 Can you provide some EDIDs?

OK, here we go.  Outputs of xrandr --verbose and EDID data from two
monitors.


thanks,

Takashi


xrandr1
Description: Binary data


xrandr2
Description: Binary data


edid1
Description: Binary data


edid2
Description: Binary data
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 10:55:16 -0400,
Adam Jackson wrote:
 
 On 4/13/12 10:29 AM, Takashi Iwai wrote:
  At Fri, 13 Apr 2012 10:14:46 -0400,
  Adam Jackson wrote:
  Yeah, that's a bug.  That's why I said it should be renamed
  drm_dmt_modes_for_range and run unconditionally if we find a range
  descriptor.
 
  Yeah, I saw your patches.  Should the further work base on them?
 
 Would be nice.
 
  Yesterday I've read a news reporting that 1366x768 is the most
  commonly used panel now, more than 1024x768.  And, 1600x900 is in the
  second place of the modern laptop panels.
 
  Windows and others do work with these resolutions on the same
  monitor.  Why Linux driver can't?  Everbody (but developers) thinks
  like that way.
 
 I think you're trying to make me defend a position I wasn't taking...

Heh, don't take it offensive.

  If it's not the native panel size and it's not a commonly found size in
  the wild, why are we obligated to provide them for every user?  Remember
  that userspace has the ability to hand in modes from above.
 
  I don't think we need to support all wild modes, too.  But the _very_
  common modes like 1366x768 and 1600x900 should be really supported as
  default.
 
 I'm not disagreeing.  I think common sizes should be available, and we 
 have code already that's intended to do that.

OK.

 My issue with the list in the patch is it contains some nonsense.  If 
 some of those more weird-looking sizes _do_ exist in the wild they 
 should be already present in EDID as the native size.  For panels where 
 they're not native I have difficulty imagining anyone wanting to set 
 that mode intentionally.  And for someone who really does want it they 
 have the ability to pass in arbitrary crap from userspace anyway.
 
 1600x900 is reasonable to add to an extras list, because it is 
 actually common despite not being in DMT.  I'm even willing to take 
 Windows as the example for what modes should be in the extras list.  But 
 I'm not willing to take I wish this was in a preset list as the sole 
 justification.

I agree with your point, too.  When I worked on supporting these modes
in X server side, I didn't pick up all such modes but only really de
facto standard ones.  It should suffice for most demands, indeed.

Also, we don't have to add always 1600x900 or 1366x768.  Such a mode
is necessary basically only when the laptop panel resolution isn't
found in the mode list.  We may add it selectively, too.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 11:30:01 -0400,
Alex Deucher wrote:
 
 On Fri, Apr 13, 2012 at 11:13 AM, Takashi Iwai ti...@suse.de wrote:
  At Fri, 13 Apr 2012 15:35:04 +0100,
  Dave Airlie wrote:
 
   I don't think we need to support all wild modes, too.  But the _very_
   common modes like 1366x768 and 1600x900 should be really supported as
   default.
 
  You guys still haven't answered the basic question, what HW is this broken?
 
  The reported problem is about HP laptops with i915 driver (no matter
  chip chip is) and several monitors with resolutions more than the
  laptop panel.
 
  The LVDS provides only the native resolution (either 1366x768 or
  1600x900) and a few other VESA ones (1024x768, 800x600 and 640x480).
  Meanwhile, the monitor EDID doesn't provide such laptop-native
  resolutions.
  Thus, in clone mode, the only possible resolution is 1024x768 or
  lower.  That's the whole problem.  It's too low and doesn't match with
  16:9 although both laptop and monitor panels are 16:9.
 
  HP wants the clone mode of the laptop-native resolution and/or a
  higher resolution with the right aspect ratio like 1280x720.  Neither
  work as of now unless you add the extra mode manually.
 
 
 One thing to be careful of is that some monitors (especially LCD
 panels) don't like modes that are not in their EDIDs.  As such when
 you try and set them you often get a wonky display or more often a
 blank screen.  We used to add a lot of inferred modes to the mode list
 in the xserver which resulted in a lot of blank screens when some odd
 mode was picked as the best match for a cloned display.  The fix was
 to only add the inferred modes on analog monitors which were more
 likely to be able to support them.

Thanks, it's good to know!

Though, I still wonder whether adding inferred modes for 1366x768 or 
1600x900 would cause any big problems.  On such monitors, 1360x768 or
1440x900 (or 1680x1050) are usually seen in the supported list.

Of course, it's never 100% safe.  But not so bad odds?


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Fri, 13 Apr 2012 12:31:25 -0400,
Adam Jackson wrote:
 
 On 4/13/12 11:41 AM, Takashi Iwai wrote:
  At Fri, 13 Apr 2012 11:30:01 -0400, Alex Deucher wrote:
  One thing to be careful of is that some monitors (especially LCD
  panels) don't like modes that are not in their EDIDs.  As such when
  you try and set them you often get a wonky display or more often a
  blank screen.  We used to add a lot of inferred modes to the mode list
  in the xserver which resulted in a lot of blank screens when some odd
  mode was picked as the best match for a cloned display.  The fix was
  to only add the inferred modes on analog monitors which were more
  likely to be able to support them.
 
  Thanks, it's good to know!
 
  Though, I still wonder whether adding inferred modes for 1366x768 or
  1600x900 would cause any big problems.  On such monitors, 1360x768 or
  1440x900 (or 1680x1050) are usually seen in the supported list.
 
  Of course, it's never 100% safe.  But not so bad odds?
 
 Mostly working is a fancy way of saying broken.

True.

 The semantics of the range descriptor are I can support modes within 
 these ranges generated by these timing formulas and/or listed in DMT. 
 That's why we only walk that mode list when we find a range descriptor: 
 if you _don't_ find a range descriptor then the monitor is explicitly 
 telling you it doesn't support arbitrary modes over a range.
 
 You can be more aggressive than that if you know your CRTC's scaler can 
 compensate, in which case you'd run the display at the native mode and 
 let the scaler translate.  The EDID parser is currently not told that 
 bit of context, and in a sense it really shouldn't; it would be a 
 function of the CRTC and the DMT list, independent of whether you have 
 EDID at all.
 
 It's not especially hard to add, I suppose.  You'd want to mark modes so 
 added so the CRTC setup knows to do the appropriate panel magic.

Right, we need some way to verify that the mode is acceptable.
It's what Rodirgo wants to figure out now...


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Wed, 11 Apr 2012 21:59:28 -0300,
Rodrigo Vivi wrote:
> 
> There are many bugs open on fd.o regarding missing modes that are supported 
> on Windows and other closed source drivers.
> >From EDID spec we can (might?) infer modes using GTF and CVT when monitor 
> >allows it trough range limited flag... obviously limiting by the range.
> >From our code:
>  * EDID spec says modes should be preferred in this order:
>  * - preferred detailed mode
>  * - other detailed modes from base block
>  * - detailed modes from extension blocks
>  * - CVT 3-byte code modes
>  * - standard timing codes
>  * - established timing codes
>  * - modes inferred from GTF or CVT range information
>  *
>  * We get this pretty much right.
> 
> Not actually so right... We were inferring just using GTF... not CVT or even 
> GTF2.
> This patch not just add some common cvt modes but also allows some modes been 
> inferred when using gtf2 as well.

I tested the patch but it doesn't detect the desired resolutions such
as 1600x900 or 1366x768, unfortunately.

Also, about the patch:


> +static const struct drm_display_mode drm_cvt_inferred_modes[] = {
> + /* 640x480 at 60Hz */
> + { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 23750  640, 664,

A missing comma here.


> +720, 800, 0, 480, 483, 487, 500, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 800x600 at 60Hz */
> + { DRM_MODE("800x600", DRM_MODE_TYPE_DRIVER, 38250, 800, 832,
> +912, 1024, 0, 600, 603, 607, 624, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 900x600 at 60Hz */
> + { DRM_MODE("900x600", DRM_MODE_TYPE_DRIVER, 45250, 960, 992,
> +1088, 1216, 0, 600, 603, 609, 624, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1024x576 at 60Hz */
> + { DRM_MODE("1024x576", DRM_MODE_TYPE_DRIVER, 46500, 1024, 1064,
> +1160, 1296, 0, 576, 579, 584, 599, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1024x768 at 60Hz */
> + { DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 63500, 1024, 1072,
> +1176, 1328, 0, 768, 771, 775, 798, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1152x864 at 60Hz */
> + { DRM_MODE("1152x864", DRM_MODE_TYPE_DRIVER, 81750, 1152, 1216,
> +1336, 1520, 0, 864, 867, 871, 897, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1280x720 at 60Hz */
> + { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74500, 1280, 1344,
> +1472, 1664, 0, 720, 723, 728, 748, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1280x768 at 60Hz */
> + { DRM_MODE("1280x768", DRM_MODE_TYPE_DRIVER, 79500, 1280, 1344,
> +1472, 1664, 0, 768, 771, 781, 798, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1280x800 at 60Hz */
> + { DRM_MODE("1280x800", DRM_MODE_TYPE_DRIVER, 83500, 1280, 1352,
> +1480, 1680, 0, 800, 803, 809, 831, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1280x1024 at 60Hz */
> + { DRM_MODE("1280x1024", DRM_MODE_TYPE_DRIVER, 109000, 1280, 1368,
> +1496, 1712, 0, 1024, 1027, 1034, 1063, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1360x768 at 60Hz */
> + { DRM_MODE("1360x768", DRM_MODE_TYPE_DRIVER, 84750, 1360, 1432,
> +1568, 1776, 0, 768, 771, 781, 798, 0,
> +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> + /* 1366x768 at 60Hz */
> + { DRM_MODE("1366x768", DRM_MODE_TYPE_DRIVER, 85250, 1368, 1440,

Here the hdisplay is 1368, not 1366.


thanks,

Takashi

Re: [Intel-gfx] [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Takashi Iwai]

At Wed, 11 Apr 2012 21:59:28 -0300,
Rodrigo Vivi wrote:
 
 There are many bugs open on fd.o regarding missing modes that are supported 
 on Windows and other closed source drivers.
 From EDID spec we can (might?) infer modes using GTF and CVT when monitor 
 allows it trough range limited flag... obviously limiting by the range.
 From our code:
  * EDID spec says modes should be preferred in this order:
  * - preferred detailed mode
  * - other detailed modes from base block
  * - detailed modes from extension blocks
  * - CVT 3-byte code modes
  * - standard timing codes
  * - established timing codes
  * - modes inferred from GTF or CVT range information
  *
  * We get this pretty much right.
 
 Not actually so right... We were inferring just using GTF... not CVT or even 
 GTF2.
 This patch not just add some common cvt modes but also allows some modes been 
 inferred when using gtf2 as well.

I tested the patch but it doesn't detect the desired resolutions such
as 1600x900 or 1366x768, unfortunately.

Also, about the patch:


 +static const struct drm_display_mode drm_cvt_inferred_modes[] = {
 + /* 640x480@60Hz */
 + { DRM_MODE(640x480, DRM_MODE_TYPE_DRIVER, 23750  640, 664,

A missing comma here.


 +720, 800, 0, 480, 483, 487, 500, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 800x600@60Hz */
 + { DRM_MODE(800x600, DRM_MODE_TYPE_DRIVER, 38250, 800, 832,
 +912, 1024, 0, 600, 603, 607, 624, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 900x600@60Hz */
 + { DRM_MODE(900x600, DRM_MODE_TYPE_DRIVER, 45250, 960, 992,
 +1088, 1216, 0, 600, 603, 609, 624, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1024x576@60Hz */
 + { DRM_MODE(1024x576, DRM_MODE_TYPE_DRIVER, 46500, 1024, 1064,
 +1160, 1296, 0, 576, 579, 584, 599, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1024x768@60Hz */
 + { DRM_MODE(1024x768, DRM_MODE_TYPE_DRIVER, 63500, 1024, 1072,
 +1176, 1328, 0, 768, 771, 775, 798, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1152x864@60Hz */
 + { DRM_MODE(1152x864, DRM_MODE_TYPE_DRIVER, 81750, 1152, 1216,
 +1336, 1520, 0, 864, 867, 871, 897, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1280x720@60Hz */
 + { DRM_MODE(1280x720, DRM_MODE_TYPE_DRIVER, 74500, 1280, 1344,
 +1472, 1664, 0, 720, 723, 728, 748, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1280x768@60Hz */
 + { DRM_MODE(1280x768, DRM_MODE_TYPE_DRIVER, 79500, 1280, 1344,
 +1472, 1664, 0, 768, 771, 781, 798, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1280x800@60Hz */
 + { DRM_MODE(1280x800, DRM_MODE_TYPE_DRIVER, 83500, 1280, 1352,
 +1480, 1680, 0, 800, 803, 809, 831, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1280x1024@60Hz */
 + { DRM_MODE(1280x1024, DRM_MODE_TYPE_DRIVER, 109000, 1280, 1368,
 +1496, 1712, 0, 1024, 1027, 1034, 1063, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1360x768@60Hz */
 + { DRM_MODE(1360x768, DRM_MODE_TYPE_DRIVER, 84750, 1360, 1432,
 +1568, 1776, 0, 768, 771, 781, 798, 0,
 +DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 + /* 1366x768@60Hz */
 + { DRM_MODE(1366x768, DRM_MODE_TYPE_DRIVER, 85250, 1368, 1440,

Here the hdisplay is 1368, not 1366.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/i915: hot removal notification to HDMI audio driver

[Takashi Iwai]

At Mon, 21 Nov 2011 09:58:09 +0800,
Wu Fengguang wrote:
> 
> On Sat, Nov 19, 2011 at 01:46:44AM +0800, Keith Packard wrote:
> > On Fri, 18 Nov 2011 17:37:40 +0800, Wu Fengguang  > intel.com> wrote:
> > 
> > > However when in X, ->mode_set won't be called at all.  Only
> > > ->get_modes and ->detect are called...
> > 
> > The desktop software will call mode_set when it configures the
> > monitor. Otherwise, it's not being used (and so shouldn't have audio
> > routed to it by default).
> 
> Keith, I experimented playing HDMI audio in X, and during the time
> unplug and plug the monitor. The HDMI audio/graphics all continue to
> work when plugged in the monitor again. Here is the dmesg showed for
> the plug event, no ->mode_set is called at all...

Which desktop system are you using?  At hotplug/unplugging, the kernel
drm issues a udev event, X Intel driver receives it and updates
Xrandr.  Then it's supposed that a daemon like gnome-settings-daemon
receives Xrandr notification and changes the modes appropriately.
Without such a background task, there will be no mode change.


Takashi

Re: [PATCH 3/3] drm/i915: hot removal notification to HDMI audio driver

[Takashi Iwai]

At Mon, 21 Nov 2011 09:58:09 +0800,
Wu Fengguang wrote:
 
 On Sat, Nov 19, 2011 at 01:46:44AM +0800, Keith Packard wrote:
  On Fri, 18 Nov 2011 17:37:40 +0800, Wu Fengguang fengguang...@intel.com 
  wrote:
  
   However when in X, -mode_set won't be called at all.  Only
   -get_modes and -detect are called...
  
  The desktop software will call mode_set when it configures the
  monitor. Otherwise, it's not being used (and so shouldn't have audio
  routed to it by default).
 
 Keith, I experimented playing HDMI audio in X, and during the time
 unplug and plug the monitor. The HDMI audio/graphics all continue to
 work when plugged in the monitor again. Here is the dmesg showed for
 the plug event, no -mode_set is called at all...

Which desktop system are you using?  At hotplug/unplugging, the kernel
drm issues a udev event, X Intel driver receives it and updates
Xrandr.  Then it's supposed that a daemon like gnome-settings-daemon
receives Xrandr notification and changes the modes appropriately.
Without such a background task, there will be no mode change.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Strange effect with i915 backlight controller

[Takashi Iwai]

At Wed, 16 Nov 2011 13:58:57 +0100,
Daniel Mack wrote:
> 
> On 11/14/2011 11:39 AM, Takashi Iwai wrote:
> > OK, then perhaps a better fix is to change the check to be equivalent
> > with pineview, as you mentioned in the original post.  The handling of
> > bit 0 for old chips was lost during the refactoring of backlight code
> > since 2.6.37.
> > 
> > Does the patch below work for you?
> > 
> > The only concern by this fix is that it changes the max value.  If
> > apps expect some certain (e.g. recorded) value, it may screw up.  But
> > I don't expect this would happen with sane apps.
> 
> Works perfectly - let's ship it :)

OK, now the patch was resent to intel-gfx with proper tags.


thanks,

Takashi

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

At Wed, 16 Nov 2011 13:58:57 +0100,
Daniel Mack wrote:
 
 On 11/14/2011 11:39 AM, Takashi Iwai wrote:
  OK, then perhaps a better fix is to change the check to be equivalent
  with pineview, as you mentioned in the original post.  The handling of
  bit 0 for old chips was lost during the refactoring of backlight code
  since 2.6.37.
  
  Does the patch below work for you?
  
  The only concern by this fix is that it changes the max value.  If
  apps expect some certain (e.g. recorded) value, it may screw up.  But
  I don't expect this would happen with sane apps.
 
 Works perfectly - let's ship it :)

OK, now the patch was resent to intel-gfx with proper tags.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Strange effect with i915 backlight controller

[Takashi Iwai]

At Mon, 14 Nov 2011 13:03:46 +0100,
Daniel Mack wrote:
> 
> On 11/14/2011 11:39 AM, Takashi Iwai wrote:
> > [Added Chris to Cc]
> > 
> > At Sun, 13 Nov 2011 17:24:09 +0100,
> > Daniel Mack wrote:
> >>
> >> Hi Takashi,
> >>
> >> On 11/10/2011 04:39 PM, Takashi Iwai wrote:
> >>> At Thu, 10 Nov 2011 16:11:29 +0100,
> >>> Daniel Mack wrote:
> >>>>
> >>>> On 11/08/2011 01:57 AM, Daniel Mack wrote:
> >>>>> Didn't get any response yet, hence copying LKML for a broader audience.
> >>>>
> >>>> Nobody, really?
> >>>>
> >>>> This is a rather annoying regression, as touching the brightness keys
> >>>> appearantly switches off the whole machine. I'm sure this is trivial to
> >>>> fix, I just don't have the insight of this driver and the chipset.
> >>>
> >>> I vaguely remember that the bit 0 is invalid on some old chips.
> >>> Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
> >>> may work.
> >>
> >> Thank you for looking into this.
> >>
> >>> ---
> >>> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> >>> b/drivers/gpu/drm/i915/intel_panel.c
> >>> index 499d4c0..be952d1 100644
> >>> --- a/drivers/gpu/drm/i915/intel_panel.c
> >>> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >>> @@ -249,8 +249,11 @@ static void 
> >>> intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
> >>>   if (IS_PINEVIEW(dev)) {
> >>>   tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
> >>>   level <<= 1;
> >>> - } else
> >>> + } else {
> >>>   tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
> >>> + if (INTEL_INFO(dev)->gen < 4)
> >>> + tmp &= ~1;
> >>> + }
> >>>   I915_WRITE(BLC_PWM_CTL, tmp | level);
> >>>  }
> >>>  
> >>
> >> This seems to be the right intention, but the value you want to modify
> >> under this condition is 'level', not 'tmp'.
> > 
> > Ah, of course.  Sorry for that.
> > 
> >> With this amendment of your
> >> patch, things work perfectly fine here.
> > 
> > OK, then perhaps a better fix is to change the check to be equivalent
> > with pineview, as you mentioned in the original post.  The handling of
> > bit 0 for old chips was lost during the refactoring of backlight code
> > since 2.6.37.
> > 
> > Does the patch below work for you?
> 
> Will test, but I only have occasional access to the machine, so this
> will have to wait for some days.

It's an old bug over a year, so no need to hurry.


Takashi

Strange effect with i915 backlight controller

[Takashi Iwai]

[Added Chris to Cc]

At Sun, 13 Nov 2011 17:24:09 +0100,
Daniel Mack wrote:
> 
> Hi Takashi,
> 
> On 11/10/2011 04:39 PM, Takashi Iwai wrote:
> > At Thu, 10 Nov 2011 16:11:29 +0100,
> > Daniel Mack wrote:
> >>
> >> On 11/08/2011 01:57 AM, Daniel Mack wrote:
> >>> Didn't get any response yet, hence copying LKML for a broader audience.
> >>
> >> Nobody, really?
> >>
> >> This is a rather annoying regression, as touching the brightness keys
> >> appearantly switches off the whole machine. I'm sure this is trivial to
> >> fix, I just don't have the insight of this driver and the chipset.
> > 
> > I vaguely remember that the bit 0 is invalid on some old chips.
> > Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
> > may work.
> 
> Thank you for looking into this.
> 
> > ---
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index 499d4c0..be952d1 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct 
> > drm_device *dev, u32 level
> > if (IS_PINEVIEW(dev)) {
> > tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
> > level <<= 1;
> > -   } else
> > +   } else {
> > tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
> > +   if (INTEL_INFO(dev)->gen < 4)
> > +   tmp &= ~1;
> > +   }
> > I915_WRITE(BLC_PWM_CTL, tmp | level);
> >  }
> >  
> 
> This seems to be the right intention, but the value you want to modify
> under this condition is 'level', not 'tmp'.

Ah, of course.  Sorry for that.

> With this amendment of your
> patch, things work perfectly fine here.

OK, then perhaps a better fix is to change the check to be equivalent
with pineview, as you mentioned in the original post.  The handling of
bit 0 for old chips was lost during the refactoring of backlight code
since 2.6.37.

Does the patch below work for you?

The only concern by this fix is that it changes the max value.  If
apps expect some certain (e.g. recorded) value, it may screw up.  But
I don't expect this would happen with sane apps.


thanks,

Takashi

===
From: Takashi Iwai <ti...@suse.de>
Subject: drm/i915: Fix invalid backpanel values for GEN3 or older chips

While refactoring of backlight control code in commit [a95735569:
drm/i915: Refactor panel backlight controls], the handling of the bit
0 of duty-cycle was gone except for pineview.  This resulted in invalid
register values for old chips like 915GM.  When the bit 0 is set, the
backlight is turned off suddenly.

This patch changes the bit-0 check by replacing with the condition of
gen < 4 (pineview is included in this condition, too).

Reported-by: Daniel Mack 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/i915/intel_panel.c |8 +++-
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..737d00f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -178,12 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
if (HAS_PCH_SPLIT(dev)) {
max >>= 16;
} else {
-   if (IS_PINEVIEW(dev)) {
+   if (INTEL_INFO(dev)->gen < 4) {
max >>= 17;
} else {
max >>= 16;
-   if (INTEL_INFO(dev)->gen < 4)
-   max &= ~1;
}

if (is_backlight_combination_mode(dev))
@@ -203,7 +201,7 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
} else {
val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
-   if (IS_PINEVIEW(dev))
+   if (INTEL_INFO(dev)->gen < 4)
val >>= 1;

if (is_backlight_combination_mode(dev)) {
@@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct 
drm_device *dev, u32 level
}

tmp = I915_READ(BLC_PWM_CTL);
-   if (IS_PINEVIEW(dev)) {
+   if (INTEL_INFO(dev)->gen < 4) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
level <<= 1;
} else
-- 
1.7.7

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

[Added Chris to Cc]

At Sun, 13 Nov 2011 17:24:09 +0100,
Daniel Mack wrote:
 
 Hi Takashi,
 
 On 11/10/2011 04:39 PM, Takashi Iwai wrote:
  At Thu, 10 Nov 2011 16:11:29 +0100,
  Daniel Mack wrote:
 
  On 11/08/2011 01:57 AM, Daniel Mack wrote:
  Didn't get any response yet, hence copying LKML for a broader audience.
 
  Nobody, really?
 
  This is a rather annoying regression, as touching the brightness keys
  appearantly switches off the whole machine. I'm sure this is trivial to
  fix, I just don't have the insight of this driver and the chipset.
  
  I vaguely remember that the bit 0 is invalid on some old chips.
  Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
  may work.
 
 Thank you for looking into this.
 
  ---
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index 499d4c0..be952d1 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct 
  drm_device *dev, u32 level
  if (IS_PINEVIEW(dev)) {
  tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
  level = 1;
  -   } else
  +   } else {
  tmp = ~BACKLIGHT_DUTY_CYCLE_MASK;
  +   if (INTEL_INFO(dev)-gen  4)
  +   tmp = ~1;
  +   }
  I915_WRITE(BLC_PWM_CTL, tmp | level);
   }
   
 
 This seems to be the right intention, but the value you want to modify
 under this condition is 'level', not 'tmp'.

Ah, of course.  Sorry for that.

 With this amendment of your
 patch, things work perfectly fine here.

OK, then perhaps a better fix is to change the check to be equivalent
with pineview, as you mentioned in the original post.  The handling of
bit 0 for old chips was lost during the refactoring of backlight code
since 2.6.37.

Does the patch below work for you?

The only concern by this fix is that it changes the max value.  If
apps expect some certain (e.g. recorded) value, it may screw up.  But
I don't expect this would happen with sane apps.


thanks,

Takashi

===
From: Takashi Iwai ti...@suse.de
Subject: drm/i915: Fix invalid backpanel values for GEN3 or older chips

While refactoring of backlight control code in commit [a95735569:
drm/i915: Refactor panel backlight controls], the handling of the bit
0 of duty-cycle was gone except for pineview.  This resulted in invalid
register values for old chips like 915GM.  When the bit 0 is set, the
backlight is turned off suddenly.

This patch changes the bit-0 check by replacing with the condition of
gen  4 (pineview is included in this condition, too).

Reported-by: Daniel Mack zon...@gmail.com
Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/drm/i915/intel_panel.c |8 +++-
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..737d00f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -178,12 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
if (HAS_PCH_SPLIT(dev)) {
max = 16;
} else {
-   if (IS_PINEVIEW(dev)) {
+   if (INTEL_INFO(dev)-gen  4) {
max = 17;
} else {
max = 16;
-   if (INTEL_INFO(dev)-gen  4)
-   max = ~1;
}
 
if (is_backlight_combination_mode(dev))
@@ -203,7 +201,7 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CPU_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
} else {
val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
-   if (IS_PINEVIEW(dev))
+   if (INTEL_INFO(dev)-gen  4)
val = 1;
 
if (is_backlight_combination_mode(dev)) {
@@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct 
drm_device *dev, u32 level
}
 
tmp = I915_READ(BLC_PWM_CTL);
-   if (IS_PINEVIEW(dev)) {
+   if (INTEL_INFO(dev)-gen  4) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
level = 1;
} else
-- 
1.7.7

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

At Mon, 14 Nov 2011 13:03:46 +0100,
Daniel Mack wrote:
 
 On 11/14/2011 11:39 AM, Takashi Iwai wrote:
  [Added Chris to Cc]
  
  At Sun, 13 Nov 2011 17:24:09 +0100,
  Daniel Mack wrote:
 
  Hi Takashi,
 
  On 11/10/2011 04:39 PM, Takashi Iwai wrote:
  At Thu, 10 Nov 2011 16:11:29 +0100,
  Daniel Mack wrote:
 
  On 11/08/2011 01:57 AM, Daniel Mack wrote:
  Didn't get any response yet, hence copying LKML for a broader audience.
 
  Nobody, really?
 
  This is a rather annoying regression, as touching the brightness keys
  appearantly switches off the whole machine. I'm sure this is trivial to
  fix, I just don't have the insight of this driver and the chipset.
 
  I vaguely remember that the bit 0 is invalid on some old chips.
  Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
  may work.
 
  Thank you for looking into this.
 
  ---
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index 499d4c0..be952d1 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -249,8 +249,11 @@ static void 
  intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
if (IS_PINEVIEW(dev)) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
level = 1;
  - } else
  + } else {
tmp = ~BACKLIGHT_DUTY_CYCLE_MASK;
  + if (INTEL_INFO(dev)-gen  4)
  + tmp = ~1;
  + }
I915_WRITE(BLC_PWM_CTL, tmp | level);
   }
   
 
  This seems to be the right intention, but the value you want to modify
  under this condition is 'level', not 'tmp'.
  
  Ah, of course.  Sorry for that.
  
  With this amendment of your
  patch, things work perfectly fine here.
  
  OK, then perhaps a better fix is to change the check to be equivalent
  with pineview, as you mentioned in the original post.  The handling of
  bit 0 for old chips was lost during the refactoring of backlight code
  since 2.6.37.
  
  Does the patch below work for you?
 
 Will test, but I only have occasional access to the machine, so this
 will have to wait for some days.

It's an old bug over a year, so no need to hurry.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Strange effect with i915 backlight controller

[Takashi Iwai]

At Thu, 10 Nov 2011 16:11:29 +0100,
Daniel Mack wrote:
> 
> On 11/08/2011 01:57 AM, Daniel Mack wrote:
> > Didn't get any response yet, hence copying LKML for a broader audience.
> 
> Nobody, really?
> 
> This is a rather annoying regression, as touching the brightness keys
> appearantly switches off the whole machine. I'm sure this is trivial to
> fix, I just don't have the insight of this driver and the chipset.

I vaguely remember that the bit 0 is invalid on some old chips.
Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
may work.


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..be952d1 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct 
drm_device *dev, u32 level
if (IS_PINEVIEW(dev)) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
level <<= 1;
-   } else
+   } else {
tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
+   if (INTEL_INFO(dev)->gen < 4)
+   tmp &= ~1;
+   }
I915_WRITE(BLC_PWM_CTL, tmp | level);
 }

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

At Thu, 10 Nov 2011 16:11:29 +0100,
Daniel Mack wrote:
 
 On 11/08/2011 01:57 AM, Daniel Mack wrote:
  Didn't get any response yet, hence copying LKML for a broader audience.
 
 Nobody, really?
 
 This is a rather annoying regression, as touching the brightness keys
 appearantly switches off the whole machine. I'm sure this is trivial to
 fix, I just don't have the insight of this driver and the chipset.

I vaguely remember that the bit 0 is invalid on some old chips.
Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
may work.


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..be952d1 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct 
drm_device *dev, u32 level
if (IS_PINEVIEW(dev)) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
level = 1;
-   } else
+   } else {
tmp = ~BACKLIGHT_DUTY_CYCLE_MASK;
+   if (INTEL_INFO(dev)-gen  4)
+   tmp = ~1;
+   }
I915_WRITE(BLC_PWM_CTL, tmp | level);
 }
 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Linux 3.2-rc1

[Takashi Iwai]

At Tue, 8 Nov 2011 12:23:30 -0800,
Linus Torvalds wrote:
> 
> Hmm, I don't know what caused this to trigger, but I'm adding both the
> i915 people and the HDA people to the cc, and they can fight to the
> death about this in the HDMI Thunderdome.

It must be the new addition of ELD-passing code.

Fengguang, can the drm or i915 driver check whether ELD is changed or
not?  Writing ELD at each time even when unchanged confuses the audio
side, as if the monitor is hotplugged.

> 
> Guys: One.. Two.. Three.. FIGHT!

Round two!


Takashi

>  Linus
> 
> On Tue, Nov 8, 2011 at 6:55 AM, Nick Bowler  
> wrote:
> >
> > Mode switches are very noisy on an Intel G45 in 3.2-rc1:
> >
> > ?HDMI hot plug event: Codec=3 Pin=3 Presence_Detect=1 ELD_Valid=1
> > ?HDMI status: Codec=3 Pin=3 Presence_Detect=1 ELD_Valid=1
> > ?HDMI: detected monitor W2253 at connection type HDMI
> > ?HDMI: available speakers: FL/FR
> > ?HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, 
> > bits = 16 20 24
> >
> > These lines get printed every single switch; previously only a single
> > line was printed once at boot (the "HDMI status" line).
> >
> > Cheers,
> > --
> > Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
> >
> 

Re: Linux 3.2-rc1

[Takashi Iwai]

At Tue, 8 Nov 2011 12:23:30 -0800,
Linus Torvalds wrote:
 
 Hmm, I don't know what caused this to trigger, but I'm adding both the
 i915 people and the HDA people to the cc, and they can fight to the
 death about this in the HDMI Thunderdome.

It must be the new addition of ELD-passing code.

Fengguang, can the drm or i915 driver check whether ELD is changed or
not?  Writing ELD at each time even when unchanged confuses the audio
side, as if the monitor is hotplugged.

 
 Guys: One.. Two.. Three.. FIGHT!

Round two!


Takashi

  Linus
 
 On Tue, Nov 8, 2011 at 6:55 AM, Nick Bowler nbow...@elliptictech.com wrote:
 
  Mode switches are very noisy on an Intel G45 in 3.2-rc1:
 
   HDMI hot plug event: Codec=3 Pin=3 Presence_Detect=1 ELD_Valid=1
   HDMI status: Codec=3 Pin=3 Presence_Detect=1 ELD_Valid=1
   HDMI: detected monitor W2253 at connection type HDMI
   HDMI: available speakers: FL/FR
   HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, 
  bits = 16 20 24
 
  These lines get printed every single switch; previously only a single
  line was printed once at boot (the HDMI status line).
 
  Cheers,
  --
  Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
 
 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

The commit 47356eb67285014527a5ab87543ba1fae3d1e10a introduced a
mechanism to record the backlight level only at disabling time, but it
also introduced a regression.  Since intel_lvds_enable() may be called
without disabling (e.g. intel_lvds_commit() calls it unconditionally),
the backlight gets back to the last recorded value.  For example, this
happens when you dim the backlight, close the lid and open the lid,
then the backlight suddenly goes to the brightest.

This patch fixes the bug by recording the backlight level always
when changed via intel_panel_set_backlight().  And,
intel_panel_{enable|disable}_backlight() call the internal function not
to update the recorded level wrongly.

Cc: 
Signed-off-by: Takashi Iwai 
---
Tested on 3.1-rc9 and 3.0.6 kernels.

v1->v2:
  make an internal intel_panel_actually_set_backlight() and call from
  enable/disable functions for tracking the level during disabled too.

 drivers/gpu/drm/i915/intel_panel.c |   21 +
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a9e0c7b..af08ff3 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -226,7 +226,7 @@ static void intel_pch_panel_set_backlight(struct drm_device 
*dev, u32 level)
I915_WRITE(BLC_PWM_CPU_CTL, val | level);
 }

-void intel_panel_set_backlight(struct drm_device *dev, u32 level)
+static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 
level)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
u32 tmp;
@@ -254,16 +254,21 @@ void intel_panel_set_backlight(struct drm_device *dev, 
u32 level)
I915_WRITE(BLC_PWM_CTL, tmp | level);
 }

-void intel_panel_disable_backlight(struct drm_device *dev)
+void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 {
struct drm_i915_private *dev_priv = dev->dev_private;

-   if (dev_priv->backlight_enabled) {
-   dev_priv->backlight_level = intel_panel_get_backlight(dev);
-   dev_priv->backlight_enabled = false;
-   }
+   dev_priv->backlight_level = level;
+   if (dev_priv->backlight_enabled)
+   intel_panel_actually_set_backlight(dev, level);
+}
+
+void intel_panel_disable_backlight(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;

-   intel_panel_set_backlight(dev, 0);
+   dev_priv->backlight_enabled = false;
+   intel_panel_actually_set_backlight(dev, 0);
 }

 void intel_panel_enable_backlight(struct drm_device *dev)
@@ -273,8 +278,8 @@ void intel_panel_enable_backlight(struct drm_device *dev)
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);

-   intel_panel_set_backlight(dev, dev_priv->backlight_level);
dev_priv->backlight_enabled = true;
+   intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
 }

 static void intel_panel_init_backlight(struct drm_device *dev)
-- 
1.7.7

[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

At Thu, 13 Oct 2011 12:28:07 -0700,
Keith Packard wrote:
> 
> On Thu, 13 Oct 2011 20:05:49 +0200, Takashi Iwai  wrote:
> 
> > Yes, this looks more understandable, indeed.
> > Would you patch it by yourself or should I refresh the patch?
> > In either way, I'll test tomorrow, as I'm already at home without a
> > test machine.
> 
> I don't have time before Monday to look at this further.

OK, I'll refresh and test the patch tomorrow.


Takashi

[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

At Thu, 13 Oct 2011 09:40:29 -0700,
Keith Packard wrote:
> 
> On Thu, 13 Oct 2011 10:57:35 +0200, Takashi Iwai  wrote:
> 
> > This patch fixes the bug by recording the backlight level always
> > when changed but only when dev_priv->backlight_enabled is set.
> > In this way, the bogus value for disabling backlight can be skipped.
> 
> I think this is better than what we have, but I'm not sure it's quite
> what we want -- the backlight level will only be remembered when it is
> enabled, so requested changes that happen while the backlight is off
> will have no effect. And, requested changes while the panel is disabled
> will still go through to the hardware, which can cause problems with
> panel power sequencing.
> 
> I think intel_panel_set_backlight should always record the level passed
> in, but that intel_panel_disable_backlight and
> intel_panel_enable_backlight should use a lower-level function, shared
> with intel_panel_set_backlight that doesn't record the value:
> 
> intel_panel_actually_set_backlight(dev, level) {
> 
> }
> 
> intel_panel_set_backlight(dev, level) {
> dev_priv->backlight_level = level;
> if (dev_priv->backlight_enabled)
> intel_panel_actually_set_backlight(dev, level);
> }
> 
> intel_panel_enable_backlight(dev) {
> dev_priv->backlight_enabled = true;
> intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
> }
> 
> intel_panel_disable_backlight(dev) {
> dev_priv->backlight_enabled = false;
> intel_panel_actually_set_backlight(dev, 0);
> }

Yes, this looks more understandable, indeed.
Would you patch it by yourself or should I refresh the patch?
In either way, I'll test tomorrow, as I'm already at home without a
test machine.


thanks,

Takashi

[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

The commit 47356eb67285014527a5ab87543ba1fae3d1e10a introduced a
mechanism to record the backlight level only at disabling time, but it
also introduced a regression.  Since intel_lvds_enable() may be called
without disabling (e.g. intel_lvds_commit() calls it unconditionally),
the backlight gets back to the last recorded value.  For example, this
happens when you dim the backlight, close the lid and open the lid,
then the backlight suddenly goes to the brightest.

This patch fixes the bug by recording the backlight level always
when changed but only when dev_priv->backlight_enabled is set.
In this way, the bogus value for disabling backlight can be skipped.

Signed-off-by: Takashi Iwai 
---

Feel free to add Cc to stable kernel, as this is a regression fix.
I've tested only a few machines here so more tests would be
appreciated, because the backlight issue is SOOO sensitive :)


 drivers/gpu/drm/i915/intel_panel.c |   11 +--
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a9e0c7b..269d526 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -233,6 +233,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 
level)

DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);

+   if (dev_priv->backlight_enabled)
+   dev_priv->backlight_level = level;
+
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);

@@ -258,11 +261,7 @@ void intel_panel_disable_backlight(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;

-   if (dev_priv->backlight_enabled) {
-   dev_priv->backlight_level = intel_panel_get_backlight(dev);
-   dev_priv->backlight_enabled = false;
-   }
-
+   dev_priv->backlight_enabled = false;
intel_panel_set_backlight(dev, 0);
 }

@@ -273,8 +272,8 @@ void intel_panel_enable_backlight(struct drm_device *dev)
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);

-   intel_panel_set_backlight(dev, dev_priv->backlight_level);
dev_priv->backlight_enabled = true;
+   intel_panel_set_backlight(dev, dev_priv->backlight_level);
 }

 static void intel_panel_init_backlight(struct drm_device *dev)
-- 
1.7.7

[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

The commit 47356eb67285014527a5ab87543ba1fae3d1e10a introduced a
mechanism to record the backlight level only at disabling time, but it
also introduced a regression.  Since intel_lvds_enable() may be called
without disabling (e.g. intel_lvds_commit() calls it unconditionally),
the backlight gets back to the last recorded value.  For example, this
happens when you dim the backlight, close the lid and open the lid,
then the backlight suddenly goes to the brightest.

This patch fixes the bug by recording the backlight level always
when changed but only when dev_priv-backlight_enabled is set.
In this way, the bogus value for disabling backlight can be skipped.

Signed-off-by: Takashi Iwai ti...@suse.de
---

Feel free to add Cc to stable kernel, as this is a regression fix.
I've tested only a few machines here so more tests would be
appreciated, because the backlight issue is SOOO sensitive :)


 drivers/gpu/drm/i915/intel_panel.c |   11 +--
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a9e0c7b..269d526 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -233,6 +233,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 
level)
 
DRM_DEBUG_DRIVER(set backlight PWM = %d\n, level);
 
+   if (dev_priv-backlight_enabled)
+   dev_priv-backlight_level = level;
+
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
 
@@ -258,11 +261,7 @@ void intel_panel_disable_backlight(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
 
-   if (dev_priv-backlight_enabled) {
-   dev_priv-backlight_level = intel_panel_get_backlight(dev);
-   dev_priv-backlight_enabled = false;
-   }
-
+   dev_priv-backlight_enabled = false;
intel_panel_set_backlight(dev, 0);
 }
 
@@ -273,8 +272,8 @@ void intel_panel_enable_backlight(struct drm_device *dev)
if (dev_priv-backlight_level == 0)
dev_priv-backlight_level = intel_panel_get_max_backlight(dev);
 
-   intel_panel_set_backlight(dev, dev_priv-backlight_level);
dev_priv-backlight_enabled = true;
+   intel_panel_set_backlight(dev, dev_priv-backlight_level);
 }
 
 static void intel_panel_init_backlight(struct drm_device *dev)
-- 
1.7.7

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

At Thu, 13 Oct 2011 09:40:29 -0700,
Keith Packard wrote:
 
 On Thu, 13 Oct 2011 10:57:35 +0200, Takashi Iwai ti...@suse.de wrote:
 
  This patch fixes the bug by recording the backlight level always
  when changed but only when dev_priv-backlight_enabled is set.
  In this way, the bogus value for disabling backlight can be skipped.
 
 I think this is better than what we have, but I'm not sure it's quite
 what we want -- the backlight level will only be remembered when it is
 enabled, so requested changes that happen while the backlight is off
 will have no effect. And, requested changes while the panel is disabled
 will still go through to the hardware, which can cause problems with
 panel power sequencing.
 
 I think intel_panel_set_backlight should always record the level passed
 in, but that intel_panel_disable_backlight and
 intel_panel_enable_backlight should use a lower-level function, shared
 with intel_panel_set_backlight that doesn't record the value:
 
 intel_panel_actually_set_backlight(dev, level) {
 internals of current intel_panel_set_backlight
 }
 
 intel_panel_set_backlight(dev, level) {
 dev_priv-backlight_level = level;
 if (dev_priv-backlight_enabled)
 intel_panel_actually_set_backlight(dev, level);
 }
 
 intel_panel_enable_backlight(dev) {
 dev_priv-backlight_enabled = true;
 intel_panel_actually_set_backlight(dev, dev_priv-backlight_level);
 }
 
 intel_panel_disable_backlight(dev) {
 dev_priv-backlight_enabled = false;
 intel_panel_actually_set_backlight(dev, 0);
 }

Yes, this looks more understandable, indeed.
Would you patch it by yourself or should I refresh the patch?
In either way, I'll test tomorrow, as I'm already at home without a
test machine.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

At Thu, 13 Oct 2011 12:28:07 -0700,
Keith Packard wrote:
 
 On Thu, 13 Oct 2011 20:05:49 +0200, Takashi Iwai ti...@suse.de wrote:
 
  Yes, this looks more understandable, indeed.
  Would you patch it by yourself or should I refresh the patch?
  In either way, I'll test tomorrow, as I'm already at home without a
  test machine.
 
 I don't have time before Monday to look at this further.

OK, I'll refresh and test the patch tomorrow.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[alsa-devel] [PATCH] pass ELD to HDMI/DP audio driver

[Takashi Iwai]

At Wed, 29 Jun 2011 14:20:13 +0800,
Wu Fengguang wrote:
> 
> This patch is tested OK on G45/HDMI and IbexPeak/HDMI. DisplayPort is
> tested on several IbexPeak and Sandybridge boxes, however not working,
> possibly due to hardware/monitor problems.

We've got some reports that DP audio doesn't work with SandyBridge
laptops although the same machines work here with a Dell monitor that
has a DP input.  Still something is flaky there...

Anyway I'll check the patch later with the machine.


thanks,

Takashi

Re: [alsa-devel] [PATCH] pass ELD to HDMI/DP audio driver

[Takashi Iwai]

At Wed, 29 Jun 2011 14:20:13 +0800,
Wu Fengguang wrote:
 
 This patch is tested OK on G45/HDMI and IbexPeak/HDMI. DisplayPort is
 tested on several IbexPeak and Sandybridge boxes, however not working,
 possibly due to hardware/monitor problems.

We've got some reports that DP audio doesn't work with SandyBridge
laptops although the same machines work here with a Dell monitor that
has a DP input.  Still something is flaky there...

Anyway I'll check the patch later with the machine.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Tue, 10 May 2011 13:08:23 +0200,
Melchior FRANZ wrote:
> 
> * Michael Chang -- Tuesday 10 May 2011:
> > Could you please try this patch and get the log ? We wonder why
> > is_backlight_combination_mode () returns false.
> 
> This information was already buried in the bugzilla thread:
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=31522
>   "It turned out that on this machine INTEL_INFO(dev)->gen equals 4,
>   and is_backlight_combination_mode() returns 0x4000."
> 
> 
> But to say it again in your words:   :-)
> 
>   [drm:is_backlight_combination_mode], BLM_COMBINATION_MODE = 1073741824  
> (0x4000)
> 
> 6x during boot-up, and several times later when changing the backlight
> brightness.
> 
> 
> This was with 8b061610dac3a3b89770c85ad63b481a47b0c38e. And now
> I have a little shocker for you (and me): because this was a
> vanilla kernel (apart from these debug messages), the screen went
> black again, like I knew it. But pressing the "brightness down"
> key turns the backlight on! I can't believe that I haven't tested
> that. I guess I've only tried "brightness up" and "display toggle".
> Those don't turn backlight on. Or maybe somethine else relevant
> meanwhile changed in the i915 drivers. (I've regularly been
> updating to HEAD.)  
> 
> So, the problem was just the initial state all the time?

Looks so, indeed.  Now, the question is what's the real cause.

IIRC, you reported that the backlight gets normal when you revert my
commit in 2.6.38.x.  So, this was regarded as a regression at first.
But, one question remains: whether the backlight level control worked
with the reverted kernel?
Judging from the attempts so far, it looks like that only LBPC can
adjust the level on your machine.  If it's true, 2.6.38.0 shouldn't be
able to adjust the level.  If you can still change the level without
LBPC, the former analysis was incorrect.

Also, with the latest 2.6.38.x, you found that the backlight gets back
when you adjust the level down.  Another question now is what happens
if you again turn it up to the max level.  Is the backlight still on?

If the backlight is kept on even with the max level, it implies that
the problem is only the initial value; once when set correctly, it'll
work fine after that.  OTOH, if the backlight gets off again at max,
it means that the max value (LBPC 0xfe) is a sort of out-of-range.
Then LBPC calculation in the driver has to be modified.


thanks,

Takashi

Re: i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Tue, 10 May 2011 13:08:23 +0200,
Melchior FRANZ wrote:
 
 * Michael Chang -- Tuesday 10 May 2011:
  Could you please try this patch and get the log ? We wonder why
  is_backlight_combination_mode () returns false.
 
 This information was already buried in the bugzilla thread:
 
   https://bugzilla.kernel.org/show_bug.cgi?id=31522
   It turned out that on this machine INTEL_INFO(dev)-gen equals 4,
   and is_backlight_combination_mode() returns 0x4000.
 
 
 But to say it again in your words:   :-)
 
   [drm:is_backlight_combination_mode], BLM_COMBINATION_MODE = 1073741824  
 (0x4000)
 
 6x during boot-up, and several times later when changing the backlight
 brightness.
 
 
 This was with 8b061610dac3a3b89770c85ad63b481a47b0c38e. And now
 I have a little shocker for you (and me): because this was a
 vanilla kernel (apart from these debug messages), the screen went
 black again, like I knew it. But pressing the brightness down
 key turns the backlight on! I can't believe that I haven't tested
 that. I guess I've only tried brightness up and display toggle.
 Those don't turn backlight on. Or maybe somethine else relevant
 meanwhile changed in the i915 drivers. (I've regularly been
 updating to HEAD.)  
 
 So, the problem was just the initial state all the time?

Looks so, indeed.  Now, the question is what's the real cause.

IIRC, you reported that the backlight gets normal when you revert my
commit in 2.6.38.x.  So, this was regarded as a regression at first.
But, one question remains: whether the backlight level control worked
with the reverted kernel?
Judging from the attempts so far, it looks like that only LBPC can
adjust the level on your machine.  If it's true, 2.6.38.0 shouldn't be
able to adjust the level.  If you can still change the level without
LBPC, the former analysis was incorrect.

Also, with the latest 2.6.38.x, you found that the backlight gets back
when you adjust the level down.  Another question now is what happens
if you again turn it up to the max level.  Is the backlight still on?

If the backlight is kept on even with the max level, it implies that
the problem is only the initial value; once when set correctly, it'll
work fine after that.  OTOH, if the backlight gets off again at max,
it means that the max value (LBPC 0xfe) is a sort of out-of-range.
Then LBPC calculation in the driver has to be modified.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Mon, 09 May 2011 02:50:50 -0600,
Joey Lee wrote:
> 
> Add Cc. Michael Chang for he is our i915 expert.
> 
> Hi Melchior, 
> 
> ? ??2011-05-08 ? 16:05 +0200?Melchior FRANZ ???
> > 
> > > Does it work to you direct control brightness by access
> > > by /sys/class/backlight/acer-wmi/brightness ?
> > 
> > No. A number written to this virtual file is accepted and remembered,
> > but it doesn't actually change the brightness. It takes setpci to do
> > that. 
> > 
> 
> I thought the video driver still is the KEY component for backlight
> issues, need fix the problem in video driver first.
> 
> > 
> >  
> > > As I remember, use setpci to control brightness is not recommended
> > > because BIOS or ACPI will also touch brightness level. That will be
> > > better control brightness by the function that was provided by BIOS. 
> > > e.g. ACPI or WMI interface, or direct control by EC.
> > 
> > Well, sounds plausible. And I wouldn't do it if it weren't the only
> > way at the moment.  :-)
> > 
> > 
> > 
> > > That means that will be better fix your Fn key control brightness like
> > > before, you just need press Fn key to change brightness and don't need
> > > have any workaround.
> > 
> > OK. I have added a lot of debug messages to intel_panel.c yesterday.
> > All it told me was that it seems to work correctly wiht acpi_osi=Linux.
> > Except that it doesn't actually change the brightness. Without acpi_osi
> > the functions aren't called at all and none of my messages showed up.
> > 
> 
> I traced _Q event in your DSDT, when acpi_osi=Linux, it run the Intel
> OpRegion logic for change brightness. And, finally, intel_opregion will
> access the function the were provided by intel_panel. So, the problem
> still back to intel_panel.
> 
> After discuss with Michael chang, we thought there have problem in your
> brightness level after add combination mode:
> 
> vi driver/gpu/drm/i915/intel_panel.c
> 
> void intel_panel_set_backlight(struct drm_device *dev, u32 level)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 tmp;
> 
> DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);
> 
> if (HAS_PCH_SPLIT(dev))
> return intel_pch_panel_set_backlight(dev, level);
> 
> if (is_backlight_combination_mode(dev)){
> u32 max = intel_panel_get_max_backlight(dev);
> u8 lbpc;
> 
> lbpc = level * 0xfe / max + 1;
> level /= lbpc;/* maybe the level 
> changed by lbpc */
> pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
> }
> 
> tmp = I915_READ(BLC_PWM_CTL);
> if (IS_PINEVIEW(dev)) {
> tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
> level <<= 1;
> } else
> tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
> I915_WRITE(BLC_PWM_CTL, tmp | level);
> }
> 
> We need to know some run time value when intel_panel_set_backlight call by 
> funciton key.

Yes, that'll help understanding.

> Please help to apply the attached debug patch to intel_panel.c then attached 
> dmesg.

The patch has an obvious typo :)
Also, we should track the value in intel_panel_get_backlight(), too.


Takashi


> > > Looks like current status is we try to fix bko#31522 but the patch
> > > causes your brightness no work by press Fn key even with acpi_osi=Linux.
> > > Does it right?
> > 
> > The history is: with acpi_osi=Linux everything worked with 2.6.38-rc8.
> > With 2.6.38 the screen stayed black. The patch that only ignored lbpc=0
> > worked (IIRC) including key adjustment. Later patches broke keys.
> > 
> > 
> > 
> > >   replace acpi_osi=Linux by acpi_osi="!Windows 2006"
> > > 
> > > Does it also works to you for backlight control?
> > 
> > No, doesn't work.
> > 
> 
> We can test the acpi_osi="!Windows 2006" again after we fix the i915's
> problem.
> 
> 
> Thank's
> Joey Lee
> 
> The following is debug patch, and please add kernel parameter
> drm.debug=0x02 :
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index f8f86e5..f62dbd9 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -236,17 +236,22 @@ void intel_panel_set_backlight(struct drm_device *dev, 
> u32 level)
>   u32 max = intel_panel_get_max_backlight(dev);
>   u8 lbpc;
>  
> + DRM_DEBUG_DRIVER("set backlight max = % lbpc = 
> level * 0xfe / max + 1;
> + DRM_DEBUG_DRIVER("set backlight lbpc = %d\n", lbpc);
>   level /= lbpc;
>   pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
>   }
>  
>   tmp = I915_READ(BLC_PWM_CTL);
> + DRM_DEBUG_DRIVER("set backlight tmp(1) = %d\n", tmp);
>   if (IS_PINEVIEW(dev)) {
>   tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
>   level <<= 1;
>   } else
>   tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
> + 

i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Sat, 7 May 2011 22:22:40 +0200,
Melchior FRANZ wrote:
> 
> * Melchior FRANZ -- Friday 06 May 2011:
> > last patch prevents the backlight from being turned off, but it also
> > breaks the brightness adjustment keys at runtime with acpi_osi=Linux.
> 
> It has turned out that acpi key events seem to be handled correctly
> and even the state of /sys/class/backlight/acer-wmi/brightness is
> updated accordingly. The only problem is that this maintained
> brightness state isn't applied to the actual backlight. It remains
> at highest level. Google pointed me to this workaround for another
> Acer notebook:
> 
>   
> https://help.ubuntu.com/community/AspireTimeline/Fixes#Alternative%20fix%20for%2010.10
> 
> This uses the acpid to write the brightness value to the display
> using setpci. And this works on my notebook as well (Acer Travelmate
> 5735Z-452G32Mnss).

Then we miss something.  With the hack above, you are doing nothing
but writing LBPC register value externally from setpci.  It implies
that the write to LBPC basically works on your machine.


Takashi

Re: i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Sat, 7 May 2011 22:22:40 +0200,
Melchior FRANZ wrote:
 
 * Melchior FRANZ -- Friday 06 May 2011:
  last patch prevents the backlight from being turned off, but it also
  breaks the brightness adjustment keys at runtime with acpi_osi=Linux.
 
 It has turned out that acpi key events seem to be handled correctly
 and even the state of /sys/class/backlight/acer-wmi/brightness is
 updated accordingly. The only problem is that this maintained
 brightness state isn't applied to the actual backlight. It remains
 at highest level. Google pointed me to this workaround for another
 Acer notebook:
 
   
 https://help.ubuntu.com/community/AspireTimeline/Fixes#Alternative%20fix%20for%2010.10
 
 This uses the acpid to write the brightness value to the display
 using setpci. And this works on my notebook as well (Acer Travelmate
 5735Z-452G32Mnss).

Then we miss something.  With the hack above, you are doing nothing
but writing LBPC register value externally from setpci.  It implies
that the write to LBPC basically works on your machine.


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: i915/kms/backlight-combo mode problem

[Takashi Iwai]

At Mon, 09 May 2011 02:50:50 -0600,
Joey Lee wrote:
 
 Add Cc. Michael Chang for he is our i915 expert.
 
 Hi Melchior, 
 
 於 日，2011-05-08 於 16:05 +0200，Melchior FRANZ 提到：
  
   Does it work to you direct control brightness by access
   by /sys/class/backlight/acer-wmi/brightness ?
  
  No. A number written to this virtual file is accepted and remembered,
  but it doesn't actually change the brightness. It takes setpci to do
  that. 
  
 
 I thought the video driver still is the KEY component for backlight
 issues, need fix the problem in video driver first.
 
  
   
   As I remember, use setpci to control brightness is not recommended
   because BIOS or ACPI will also touch brightness level. That will be
   better control brightness by the function that was provided by BIOS. 
   e.g. ACPI or WMI interface, or direct control by EC.
  
  Well, sounds plausible. And I wouldn't do it if it weren't the only
  way at the moment.  :-)
  
  
  
   That means that will be better fix your Fn key control brightness like
   before, you just need press Fn key to change brightness and don't need
   have any workaround.
  
  OK. I have added a lot of debug messages to intel_panel.c yesterday.
  All it told me was that it seems to work correctly wiht acpi_osi=Linux.
  Except that it doesn't actually change the brightness. Without acpi_osi
  the functions aren't called at all and none of my messages showed up.
  
 
 I traced _Q event in your DSDT, when acpi_osi=Linux, it run the Intel
 OpRegion logic for change brightness. And, finally, intel_opregion will
 access the function the were provided by intel_panel. So, the problem
 still back to intel_panel.
 
 After discuss with Michael chang, we thought there have problem in your
 brightness level after add combination mode:
 
 vi driver/gpu/drm/i915/intel_panel.c
 
 void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 {
 struct drm_i915_private *dev_priv = dev-dev_private;
 u32 tmp;
 
 DRM_DEBUG_DRIVER(set backlight PWM = %d\n, level);
 
 if (HAS_PCH_SPLIT(dev))
 return intel_pch_panel_set_backlight(dev, level);
 
 if (is_backlight_combination_mode(dev)){
 u32 max = intel_panel_get_max_backlight(dev);
 u8 lbpc;
 
 lbpc = level * 0xfe / max + 1;
 level /= lbpc;/* maybe the level 
 changed by lbpc */
 pci_write_config_byte(dev-pdev, PCI_LBPC, lbpc);
 }
 
 tmp = I915_READ(BLC_PWM_CTL);
 if (IS_PINEVIEW(dev)) {
 tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
 level = 1;
 } else
 tmp = ~BACKLIGHT_DUTY_CYCLE_MASK;
 I915_WRITE(BLC_PWM_CTL, tmp | level);
 }
 
 We need to know some run time value when intel_panel_set_backlight call by 
 funciton key.

Yes, that'll help understanding.

 Please help to apply the attached debug patch to intel_panel.c then attached 
 dmesg.

The patch has an obvious typo :)
Also, we should track the value in intel_panel_get_backlight(), too.


Takashi


   Looks like current status is we try to fix bko#31522 but the patch
   causes your brightness no work by press Fn key even with acpi_osi=Linux.
   Does it right?
  
  The history is: with acpi_osi=Linux everything worked with 2.6.38-rc8.
  With 2.6.38 the screen stayed black. The patch that only ignored lbpc=0
  worked (IIRC) including key adjustment. Later patches broke keys.
  
  
  
 replace acpi_osi=Linux by acpi_osi=!Windows 2006
   
   Does it also works to you for backlight control?
  
  No, doesn't work.
  
 
 We can test the acpi_osi=!Windows 2006 again after we fix the i915's
 problem.
 
 
 Thank's
 Joey Lee
 
 The following is debug patch, and please add kernel parameter
 drm.debug=0x02 :
 
 diff --git a/drivers/gpu/drm/i915/intel_panel.c 
 b/drivers/gpu/drm/i915/intel_panel.c
 index f8f86e5..f62dbd9 100644
 --- a/drivers/gpu/drm/i915/intel_panel.c
 +++ b/drivers/gpu/drm/i915/intel_panel.c
 @@ -236,17 +236,22 @@ void intel_panel_set_backlight(struct drm_device *dev, 
 u32 level)
   u32 max = intel_panel_get_max_backlight(dev);
   u8 lbpc;
  
 + DRM_DEBUG_DRIVER(set backlight max = % lbpc = 
 level * 0xfe / max + 1;
 + DRM_DEBUG_DRIVER(set backlight lbpc = %d\n, lbpc);
   level /= lbpc;
   pci_write_config_byte(dev-pdev, PCI_LBPC, lbpc);
   }
  
   tmp = I915_READ(BLC_PWM_CTL);
 + DRM_DEBUG_DRIVER(set backlight tmp(1) = %d\n, tmp);
   if (IS_PINEVIEW(dev)) {
   tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
   level = 1;
   } else
   tmp = ~BACKLIGHT_DUTY_CYCLE_MASK;
 + DRM_DEBUG_DRIVER(set backlight tmp(2) = %d\n, tmp);
 + DRM_DEBUG_DRIVER(set backlight level = %d\n, level);
   I915_WRITE(BLC_PWM_CTL, tmp | level);
  }
  
 
 
___
dri-devel mailing 

i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Sat, 30 Apr 2011 13:34:51 +0200,
Melchior FRANZ wrote:
> 
> Dropping Linus from the CC.
> 
> 
> * Takashi Iwai -- Saturday 30 April 2011:
> * * At Sat, 30 Apr 2011 10:32:04 +0200, Melchior FRANZ wrote:
> > > Yes, backlight adjustment generally works on this notebook, but only
> > > with "acpi_osi=Linux" on the command line.
> > 
> > acpi_osi quirk should be better added statically, then.
> 
> No, I guess the problem here is that acer_wmi doesn't support this
> machine yet. I told the ACER WMI maintainer and sent a DSDT image,
> but this message was thoroughly ignored.
> 
> 
>   $ dmesg|grep -i acer
>   [0.00] DMI: Acer TM5735/BA51_MV, BIOS V1.04 09/23/2010
>   [   71.850534] acer_wmi: Acer Laptop ACPI-WMI Extras
>   [   72.350278] acer_wmi: Unable to detect available WMID devices
>   
>   Machine: Acer Travelmate 5735Z-452G32Mnss

Hm, but the backlight control is done via the standard ACPI, no?
If so, the fact that acer_wmi doesn't work sounds irrelevant.

BTW, did you try my previous patch?


Takashi

Re: i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Sat, 30 Apr 2011 13:34:51 +0200,
Melchior FRANZ wrote:
 
 Dropping Linus from the CC.
 
 
 * Takashi Iwai -- Saturday 30 April 2011:
 * * At Sat, 30 Apr 2011 10:32:04 +0200, Melchior FRANZ wrote:
   Yes, backlight adjustment generally works on this notebook, but only
   with acpi_osi=Linux on the command line.
  
  acpi_osi quirk should be better added statically, then.
 
 No, I guess the problem here is that acer_wmi doesn't support this
 machine yet. I told the ACER WMI maintainer and sent a DSDT image,
 but this message was thoroughly ignored.
 
 
   $ dmesg|grep -i acer
   [0.00] DMI: Acer TM5735/BA51_MV, BIOS V1.04 09/23/2010
   [   71.850534] acer_wmi: Acer Laptop ACPI-WMI Extras
   [   72.350278] acer_wmi: Unable to detect available WMID devices
   
   Machine: Acer Travelmate 5735Z-452G32Mnss

Hm, but the backlight control is done via the standard ACPI, no?
If so, the fact that acer_wmi doesn't work sounds irrelevant.

BTW, did you try my previous patch?


Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Sat, 30 Apr 2011 10:32:04 +0200,
Melchior FRANZ wrote:
> 
> * Takashi Iwai -- Saturday 30 April 2011:
> > I remember vaguely that the value zero could be interpreted as the max.
> 
> > Also, with the patch, does the backlight level can be adjusted
> > correctly to different values?  I wonder whether LBPC adjustment
> > really works or not on your machine.
> 
> > +   if (!lbpc)
> > +   lbpc = 0xff; /* max value */
> 
> This change does *not* work on my machine. The screen stays black
> again.

Hrm, then it's really hard to say how exactly the system behaves when
lbpc=0...  I guess we should avoid controlling LBPC in such a case,
e.g. something like the patch below (totally untested).

But Intel guys must know of this better... I leave this to their
hands.

> 
> Yes, backlight adjustment generally works on this notebook, but only
> with "acpi_osi=Linux" on the command line.

acpi_osi quirk should be better added statically, then.


thanks,

Takashi

---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1c1b27c..c0ab771 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -328,6 +328,7 @@ typedef struct drm_i915_private {
struct intel_overlay *overlay;

/* LVDS info */
+   int backlight_combination_mode; /* 0=unknown, -1=no, 1=yes */
int backlight_level;  /* restore backlight to this value */
bool backlight_enabled;
struct drm_display_mode *panel_fixed_mode;
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..320dd5f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -115,14 +115,24 @@ done:
 static int is_backlight_combination_mode(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
-
-   if (INTEL_INFO(dev)->gen >= 4)
-   return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
-
-   if (IS_GEN2(dev))
-   return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
-
-   return 0;
+   int combo_mode;
+   u8 lbpc;
+
+   if (dev_priv->backlight_combination_mode)
+   return dev_priv->backlight_combination_mode > 0;
+
+   pci_read_config_byte(dev->pdev, PCI_LBPC, );
+   if (!lbpc)
+   combo_mode = 0;
+   else if (INTEL_INFO(dev)->gen >= 4)
+   combo_mode = I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
+   else if (IS_GEN2(dev))
+   combo_mode = I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
+   else
+   combo_mode = 0;
+
+   dev_priv->backlight_combination_mode = combo_mode ? -1 : 1;
+   return combo_mode;
 }

 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)

i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Fri, 29 Apr 2011 22:09:54 +0200,
Melchior FRANZ wrote:
> 
> * Takashi Iwai -- Friday 29 April 2011:
> [https://bugzilla.kernel.org/show_bug.cgi?id=31522]
> > Looking at bugzilla, the problem seems like the case lbpc=0.
> > What about the patch below instead?
> 
> > -   val *= lbpc;
> > +   if (lbpc)
> > +   val *= lbpc;
> 
> Yes, that works as well. (I had assumed that this was the problem,
> but wasn't sure if a zero was even a legitimate value, or rather
> a sign for a problem or wrong assumption elsewhere.)

Well, this was just my wild guess.  I remember vaguely that the value
zero could be interpreted as the max.  It might be depending on the
hardware.

So, the patch below may work better in your case.

Also, with the patch, does the backlight level can be adjusted
correctly to different values?  I wonder whether LBPC adjustment
really works or not on your machine.


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..aaa1f9e 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -207,6 +207,8 @@ u32 intel_panel_get_backlight(struct drm_device *dev)

val &= ~1;
pci_read_config_byte(dev->pdev, PCI_LBPC, );
+   if (!lbpc)
+   lbpc = 0xff; /* max value */
val *= lbpc;
}
}

Re: i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Fri, 29 Apr 2011 22:09:54 +0200,
Melchior FRANZ wrote:
 
 * Takashi Iwai -- Friday 29 April 2011:
 [https://bugzilla.kernel.org/show_bug.cgi?id=31522]
  Looking at bugzilla, the problem seems like the case lbpc=0.
  What about the patch below instead?
 
  -   val *= lbpc;
  +   if (lbpc)
  +   val *= lbpc;
 
 Yes, that works as well. (I had assumed that this was the problem,
 but wasn't sure if a zero was even a legitimate value, or rather
 a sign for a problem or wrong assumption elsewhere.)

Well, this was just my wild guess.  I remember vaguely that the value
zero could be interpreted as the max.  It might be depending on the
hardware.

So, the patch below may work better in your case.

Also, with the patch, does the backlight level can be adjusted
correctly to different values?  I wonder whether LBPC adjustment
really works or not on your machine.


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..aaa1f9e 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -207,6 +207,8 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 
val = ~1;
pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
+   if (!lbpc)
+   lbpc = 0xff; /* max value */
val *= lbpc;
}
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Sat, 30 Apr 2011 10:32:04 +0200,
Melchior FRANZ wrote:
 
 * Takashi Iwai -- Saturday 30 April 2011:
  I remember vaguely that the value zero could be interpreted as the max.
 
  Also, with the patch, does the backlight level can be adjusted
  correctly to different values?  I wonder whether LBPC adjustment
  really works or not on your machine.
 
  +   if (!lbpc)
  +   lbpc = 0xff; /* max value */
 
 This change does *not* work on my machine. The screen stays black
 again.

Hrm, then it's really hard to say how exactly the system behaves when
lbpc=0...  I guess we should avoid controlling LBPC in such a case,
e.g. something like the patch below (totally untested).

But Intel guys must know of this better... I leave this to their
hands.

 
 Yes, backlight adjustment generally works on this notebook, but only
 with acpi_osi=Linux on the command line.

acpi_osi quirk should be better added statically, then.


thanks,

Takashi

---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1c1b27c..c0ab771 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -328,6 +328,7 @@ typedef struct drm_i915_private {
struct intel_overlay *overlay;
 
/* LVDS info */
+   int backlight_combination_mode; /* 0=unknown, -1=no, 1=yes */
int backlight_level;  /* restore backlight to this value */
bool backlight_enabled;
struct drm_display_mode *panel_fixed_mode;
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..320dd5f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -115,14 +115,24 @@ done:
 static int is_backlight_combination_mode(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
-
-   if (INTEL_INFO(dev)-gen = 4)
-   return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
-
-   if (IS_GEN2(dev))
-   return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
-
-   return 0;
+   int combo_mode;
+   u8 lbpc;
+
+   if (dev_priv-backlight_combination_mode)
+   return dev_priv-backlight_combination_mode  0;
+
+   pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
+   if (!lbpc)
+   combo_mode = 0;
+   else if (INTEL_INFO(dev)-gen = 4)
+   combo_mode = I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
+   else if (IS_GEN2(dev))
+   combo_mode = I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
+   else
+   combo_mode = 0;
+
+   dev_priv-backlight_combination_mode = combo_mode ? -1 : 1;
+   return combo_mode;
 }
 
 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Fri, 29 Apr 2011 19:41:53 +0200,
Melchior FRANZ wrote:
> 
> * Takashi Iwai -- Friday 29 April 2011:
> > Melchior FRANZ wrote:
> > > The bug was introduced with commit 
> > > ba3820ade317ee36e496b9b40d2ec3987dd4aef0
> > > [...] when using KMS my notebook's[2] screen remains dark, because the
> > > backlight isn't turned on.
> 
> > Could you check whether the patch below changes the behavior?
> > If this cures, it means that the backlight-combo mode doesn't work on
> > your machine.
> 
> Yes, that works. (Test was with fafc9929c668f8bae6dd1f109f33a86d2cb3c460,
> which is current HEAD.)

Looking at bugzilla, the problem seems like the case lbpc=0.
What about the patch below instead?


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..ba60218 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -207,7 +207,8 @@ u32 intel_panel_get_backlight(struct drm_device *dev)

val &= ~1;
pci_read_config_byte(dev->pdev, PCI_LBPC, );
-   val *= lbpc;
+   if (lbpc)
+   val *= lbpc;
}
}

Linux 2.6.39-rc5

[Takashi Iwai]

At Fri, 29 Apr 2011 17:58:06 +0200,
Melchior FRANZ wrote:
> 
> * Linus Torvalds -- Wednesday 27 April 2011:
> > Go forth and test,
> 
> Doesn't work on my notebook with i915/KMS. But then again, neither did
> 2.6.38 or any of its stable releases. The last working version was
> 2.6.38-rc8. The problem has been reported[1], but the bug got closed
> in the wrong assumption that the bug is fixed. I reported that, too.
> No reactions to that.
> 
> The bug was introduced with commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0
> by Takashi Iwai. The result is that when using KMS my notebook's[2] screen
> remains dark, because the backlight isn't turned on.
> 
> Reverting commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0 makes my
> notebook work correctly again as it used to in 2.6.38-rc8 and before.

Could you check whether the patch below changes the behavior?
If this cures, it means that the backlight-combo mode doesn't work on
your machine.


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..bf61e02 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -116,11 +116,13 @@ static int is_backlight_combination_mode(struct 
drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;

+#if 0
if (INTEL_INFO(dev)->gen >= 4)
return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;

if (IS_GEN2(dev))
return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
+#endif

return 0;
 }

Re: i915/kms/backlight-combo mode problem (was: Re: Linux 2.6.39-rc5)

[Takashi Iwai]

At Fri, 29 Apr 2011 19:41:53 +0200,
Melchior FRANZ wrote:
 
 * Takashi Iwai -- Friday 29 April 2011:
  Melchior FRANZ wrote:
   The bug was introduced with commit 
   ba3820ade317ee36e496b9b40d2ec3987dd4aef0
   [...] when using KMS my notebook's[2] screen remains dark, because the
   backlight isn't turned on.
 
  Could you check whether the patch below changes the behavior?
  If this cures, it means that the backlight-combo mode doesn't work on
  your machine.
 
 Yes, that works. (Test was with fafc9929c668f8bae6dd1f109f33a86d2cb3c460,
 which is current HEAD.)

Looking at bugzilla, the problem seems like the case lbpc=0.
What about the patch below instead?


Takashi

---
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..ba60218 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -207,7 +207,8 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 
val = ~1;
pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
-   val *= lbpc;
+   if (lbpc)
+   val *= lbpc;
}
}
 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Recover DPMS properly after XRandr re-enablement

[Takashi Iwai]

At Sun, 17 Apr 2011 18:26:54 +0200,
Florian Mickler wrote:
> 
> On Wed, 22 Dec 2010 12:53:07 +
> Chris Wilson  wrote:
> 
> > On Wed, 22 Dec 2010 12:42:32 +, Chris Wilson  > chris-wilson.co.uk> wrote:
> > > From: Takashi Iwai 
> > 
> > > This patch adds a new helper function to manage the drm_connector
> > > DPMS so that it can be called commonly in both places.
> > > 
> > > Signed-off-by: Takashi Iwai 
> > 
> > FWIW,
> > 
> > Reviewed-by: Chris Wilson 
> > 
> > -Chris
> > 
> 
> Is this patch still necessary for 2.6.39?

Yes, I think so.


Takashi

> It was needed to fix a regression from 2.6.36 (at least in February). 
> ( https://bugzilla.kernel.org/show_bug.cgi?id=24982 )
> 
> Regards,
> Flo
> 

Re: [PATCH] drm: Recover DPMS properly after XRandr re-enablement

[Takashi Iwai]

At Sun, 17 Apr 2011 18:26:54 +0200,
Florian Mickler wrote:
 
 On Wed, 22 Dec 2010 12:53:07 +
 Chris Wilson ch...@chris-wilson.co.uk wrote:
 
  On Wed, 22 Dec 2010 12:42:32 +, Chris Wilson ch...@chris-wilson.co.uk 
  wrote:
   From: Takashi Iwai ti...@suse.de
  
   This patch adds a new helper function to manage the drm_connector
   DPMS so that it can be called commonly in both places.
   
   Signed-off-by: Takashi Iwai ti...@suse.de
  
  FWIW,
  
  Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
  
  -Chris
  
 
 Is this patch still necessary for 2.6.39?

Yes, I think so.


Takashi

 It was needed to fix a regression from 2.6.36 (at least in February). 
 ( https://bugzilla.kernel.org/show_bug.cgi?id=24982 )
 
 Regards,
 Flo
 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: Revive combination mode for backlight control

[Takashi Iwai]

At Fri, 11 Mar 2011 02:23:08 +0100 (CET),
Indan Zupancic wrote:
> 
> Hi,
> 
> Some nitpicks below. I know you're just restoring the original code,
> but if we improve it we can as well do it now.

Well, Linus already merged my patch quickly.  So, we can improve the
readability as an additional patch, but I think it's likely a 2.6.39
material.

All you comments below look reasonable.  Could you send a new patch?


thanks,

Takashi


> On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
> > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> >
> > drm/i915: Do not handle backlight combination mode specially
> >
> > since this commit introduced other regressions due to untouched LBPC
> > register, e.g. the backlight dimmed after resume.
> 
> The regression was that, if ALSE was used, the maximum brightness
> would be the brightness at boot, because LBPC isn't touched and
> the BIOS restores it at boot. So the sympom would be less brightness
> levels or lower maximum brightness.
> 
> Systems with no ASLE support work fine because there the code is only
> used to save and restore the PWM register. LBPC is saved and restored
> by the BIOS.
> 
> The backlight dimming after resume/blanking was the original bug
> caused by the bogus val <<=1 shift.
> 
> > In addition to the revert, this patch includes a fix for the original
> > issue (weird backlight levels) by removing the wrong bit shift for
> > computing the current backlight level.
> > Also, including typo fixes (lpbc -> lbpc).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
> > Acked-by: Indan Zupancic 
> > Cc: 
> > Signed-off-by: Takashi Iwai 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h|   10 ++
> >  drivers/gpu/drm/i915/intel_panel.c |   36 
> > 
> >  2 files changed, 46 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 3e6f486..2abe240 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1553,7 +1553,17 @@
> >
> >  /* Backlight control */
> >  #define BLC_PWM_CTL0x61254
> > +#define   BACKLIGHT_MODULATION_FREQ_SHIFT  (17)
> 
> This one isn't used anywhere.
> 
> >  #define BLC_PWM_CTL2   0x61250 /* 965+ only */
> > +#define   BLM_COMBINATION_MODE (1 << 30)
> > +/*
> > + * This is the most significant 15 bits of the number of backlight cycles 
> > in a
> > + * complete cycle of the modulated backlight control.
> > + *
> > + * The actual value is this field multiplied by two.
> > + */
> > +#define   BACKLIGHT_MODULATION_FREQ_MASK   (0x7fff << 17)
> 
> This one isn't used and the comment is misleading, I think.
> 
> > +#define   BLM_LEGACY_MODE  (1 << 16)
> >  /*
> >   * This is the number of cycles out of the backlight modulation cycle for 
> > which
> >   * the backlight is on.
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index d860abe..f8f86e5 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -30,6 +30,8 @@
> >
> >  #include "intel_drv.h"
> >
> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> > +
> 
> I'd either put all the defines in i915_reg.h, or have all combination
> mode specific defines here. Though I guess LBPC is an odd one.
> 
> >  void
> >  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >struct drm_display_mode *adjusted_mode)
> > @@ -110,6 +112,19 @@ done:
> > dev_priv->pch_pf_size = (width << 16) | height;
> >  }
> >
> > +static int is_backlight_combination_mode(struct drm_device *dev)
> > +{
> > +   struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +   if (INTEL_INFO(dev)->gen >= 4)
> > +   return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> > +
> > +   if (IS_GEN2(dev))
> > +   return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> > +
> > +   return 0;
> > +}
> > +
> >  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >  {
> > u32 val;
> > @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device 
> > *dev)
> > if (INTEL_INFO(dev)->gen < 4)
> >

drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Takashi Iwai]

[Removed stable-kernel from Cc]

At Fri, 11 Mar 2011 02:35:45 +0100 (CET),
Indan Zupancic wrote:
> 
> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
> 
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored at resume time.
> 
> Nothing guarantees that those calls will be balanced, so having
> backlight_enabled makes no sense, as the real state can change
> without the panel code noticing. So keep things as stateless as
> possible.
> 
> Signed-off-by: Indan Zupancic 

Indan, when you need a patch to be added to stable kernel, just put
Cc to stable kernel around your sign-off line.  Then Greg will pick it
up automatically once when the patch is merged to Linus tree.

Anyway, the patch looks good to me.  A nice clean-up.

Reviewed-by: Takashi Iwai 


thanks,

Takashi

[PATCH] drm/i915: Revive combination mode for backlight control

[Takashi Iwai]

This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb

drm/i915: Do not handle backlight combination mode specially

since this commit introduced other regressions due to untouched LBPC
register, e.g. the backlight dimmed after resume.

In addition to the revert, this patch includes a fix for the original
issue (weird backlight levels) by removing the wrong bit shift for
computing the current backlight level.
Also, including typo fixes (lpbc -> lbpc).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
Acked-by: Indan Zupancic 
Cc: 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/i915/i915_reg.h|   10 ++
 drivers/gpu/drm/i915/intel_panel.c |   36 
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3e6f486..2abe240 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1553,7 +1553,17 @@

 /* Backlight control */
 #define BLC_PWM_CTL0x61254
+#define   BACKLIGHT_MODULATION_FREQ_SHIFT  (17)
 #define BLC_PWM_CTL2   0x61250 /* 965+ only */
+#define   BLM_COMBINATION_MODE (1 << 30)
+/*
+ * This is the most significant 15 bits of the number of backlight cycles in a
+ * complete cycle of the modulated backlight control.
+ *
+ * The actual value is this field multiplied by two.
+ */
+#define   BACKLIGHT_MODULATION_FREQ_MASK   (0x7fff << 17)
+#define   BLM_LEGACY_MODE  (1 << 16)
 /*
  * This is the number of cycles out of the backlight modulation cycle for which
  * the backlight is on.
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..f8f86e5 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,6 +30,8 @@

 #include "intel_drv.h"

+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
+
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
   struct drm_display_mode *adjusted_mode)
@@ -110,6 +112,19 @@ done:
dev_priv->pch_pf_size = (width << 16) | height;
 }

+static int is_backlight_combination_mode(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+
+   if (INTEL_INFO(dev)->gen >= 4)
+   return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
+
+   if (IS_GEN2(dev))
+   return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
+
+   return 0;
+}
+
 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
 {
u32 val;
@@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
if (INTEL_INFO(dev)->gen < 4)
max &= ~1;
}
+
+   if (is_backlight_combination_mode(dev))
+   max *= 0xff;
}

DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +201,14 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val >>= 1;
+
+   if (is_backlight_combination_mode(dev)){
+   u8 lbpc;
+
+   val &= ~1;
+   pci_read_config_byte(dev->pdev, PCI_LBPC, );
+   val *= lbpc;
+   }
}

DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +231,16 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 
level)

if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
+
+   if (is_backlight_combination_mode(dev)){
+   u32 max = intel_panel_get_max_backlight(dev);
+   u8 lbpc;
+
+   lbpc = level * 0xfe / max + 1;
+   level /= lbpc;
+   pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
+   }
+
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
-- 
1.7.4.1

[PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
> 
> On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 08:49:37 +0100,
> > Takashi Iwai wrote:
> >>
> >> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> >> Indan Zupancic wrote:
> >> >
> >> > Hello,
> >> >
> >> > On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> >> > > Alex, can you confirm that the revert of 951f3512dba5 plus the
> >> > > one-liner patch from Takashi that Indan quoted also works for you?
> >> > >
> >> > >   Linus
> >> > >
> >> > > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic  
> >> > > wrote:
> >> > >>
> >> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> > >> most immediate bug without changing anything else. This should go
> >> > >> in stable too.
> >> > >
> >> >
> >> > I found another backlight bug:
> >> >
> >> > When suspending intel_panel_disable_backlight() is never called,
> >> > but intel_panel_enable_backlight() is called at resume. With the
> >> > effect that if the brightness was ever changed after screen
> >> > blanking, the wrong brightness gets restored.
> >> >
> >> > This explains the weird behaviour I've seen. I didn't see it with
> >> > combination mode, because then the brightness is always the same
> >> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> >> > send a patch in a moment.
> >> >
> >> > Alternative for reverting the combination mode removal (I can also
> >> > redo the patch against the revert and Takashi's patch, if that's
> >> > preferred):
> >> >
> >> > --
> >> >
> >> > drm/i915: Do handle backlight combination mode specially
> >> >
> >> > Add back the combination mode check, but with slightly cleaner code
> >> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> >> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> >> > The other change is clearer calculations: Just check for zero level
> >> > explicitly instead of avoiding the divide-by-zero.
> >> >
> >> > Signed-off-by: Indan Zupancic 
> >> >
> >> > ---
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> >> > b/drivers/gpu/drm/i915/intel_panel.c
> >> > index d860abe..b05631a 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -30,6 +30,10 @@
> >> >
> >> >  #include "intel_drv.h"
> >> >
> >> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> >> > +#define BLM_COMBINATION_MODE (1 << 30)
> >> > +#define BLM_LEGACY_MODE (1 << 16)
> >> > +
> >> >  void
> >> >  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >> > struct drm_display_mode *adjusted_mode)
> >> > @@ -110,6 +114,22 @@ done:
> >> >  dev_priv->pch_pf_size = (width << 16) | height;
> >> >  }
> >> >
> >> > +/*
> >> > + * What about gen 3? If there are no gen 3 systems with ASLE,
> >> > + * then it doesn't matter, as we don't need to change the
> >> > + * brightness. But then the gen 2 check can be removed too.
> >> > + */
> >> > +static int is_backlight_combination_mode(struct drm_device *dev)
> >> > +{
> >> > +struct drm_i915_private *dev_priv = dev->dev_private;
> >> > +
> >> > +if (INTEL_INFO(dev)->gen >= 4)
> >> > +return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> >> > +if (IS_GEN2(dev))
> >> > +return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> >> > +return 0;
> >> > +}
> >> > +
> >> >  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >> >  {
> >> >  u32 val;
> >> > @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
> >> > *dev)
> >> >  max >

[PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 09:45:18 +0100 (CET),
Indan Zupancic wrote:
> 
> On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> > Indan Zupancic wrote:
> >>
> >> Hello,
> >>
> >> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> >> > Alex, can you confirm that the revert of 951f3512dba5 plus the
> >> > one-liner patch from Takashi that Indan quoted also works for you?
> >> >
> >> >   Linus
> >> >
> >> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic  wrote:
> >> >>
> >> >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> >> most immediate bug without changing anything else. This should go
> >> >> in stable too.
> >> >
> >>
> >> I found another backlight bug:
> >>
> >> When suspending intel_panel_disable_backlight() is never called,
> >> but intel_panel_enable_backlight() is called at resume. With the
> >> effect that if the brightness was ever changed after screen
> >> blanking, the wrong brightness gets restored.
> >>
> >> This explains the weird behaviour I've seen. I didn't see it with
> >> combination mode, because then the brightness is always the same
> >> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> >> send a patch in a moment.
> >>
> >> Alternative for reverting the combination mode removal (I can also
> >> redo the patch against the revert and Takashi's patch, if that's
> >> preferred):
> >>
> >> --
> >>
> >> drm/i915: Do handle backlight combination mode specially
> >>
> >> Add back the combination mode check, but with slightly cleaner code
> >> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> >> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> >> The other change is clearer calculations: Just check for zero level
> >> explicitly instead of avoiding the divide-by-zero.
> >>
> >> Signed-off-by: Indan Zupancic 
> >>
> >> ---
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> >> b/drivers/gpu/drm/i915/intel_panel.c
> >> index d860abe..b05631a 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -30,6 +30,10 @@
> >>
> >>  #include "intel_drv.h"
> >>
> >> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> >> +#define BLM_COMBINATION_MODE (1 << 30)
> >> +#define BLM_LEGACY_MODE (1 << 16)
> >> +
> >>  void
> >>  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >>   struct drm_display_mode *adjusted_mode)
> >> @@ -110,6 +114,22 @@ done:
> >>dev_priv->pch_pf_size = (width << 16) | height;
> >>  }
> >>
> >> +/*
> >> + * What about gen 3? If there are no gen 3 systems with ASLE,
> >> + * then it doesn't matter, as we don't need to change the
> >> + * brightness. But then the gen 2 check can be removed too.
> >> + */
> >> +static int is_backlight_combination_mode(struct drm_device *dev)
> >> +{
> >> +  struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +  if (INTEL_INFO(dev)->gen >= 4)
> >> +  return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> >> +  if (IS_GEN2(dev))
> >> +  return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> >> +  return 0;
> >> +}
> >> +
> >>  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >>  {
> >>u32 val;
> >> @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
> >> *dev)
> >>max >>= 17;
> >>} else {
> >>max >>= 16;
> >> +  /* Ignore BLM_LEGACY_MODE bit */
> >>if (INTEL_INFO(dev)->gen < 4)
> >>max &= ~1;
> >>}
> >> +  if (is_backlight_combination_mode(dev))
> >> +  max *= 0xff;
> >>}
> >>
> >>DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> >> @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> >>val =

[PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 08:49:37 +0100,
Takashi Iwai wrote:
> 
> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> Indan Zupancic wrote:
> > 
> > Hello,
> > 
> > On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> > > Alex, can you confirm that the revert of 951f3512dba5 plus the
> > > one-liner patch from Takashi that Indan quoted also works for you?
> > >
> > >   Linus
> > >
> > > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic  wrote:
> > >>
> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
> > >> most immediate bug without changing anything else. This should go
> > >> in stable too.
> > >
> > 
> > I found another backlight bug:
> > 
> > When suspending intel_panel_disable_backlight() is never called,
> > but intel_panel_enable_backlight() is called at resume. With the
> > effect that if the brightness was ever changed after screen
> > blanking, the wrong brightness gets restored.
> > 
> > This explains the weird behaviour I've seen. I didn't see it with
> > combination mode, because then the brightness is always the same
> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> > send a patch in a moment.
> > 
> > Alternative for reverting the combination mode removal (I can also
> > redo the patch against the revert and Takashi's patch, if that's
> > preferred):
> > 
> > --
> > 
> > drm/i915: Do handle backlight combination mode specially
> > 
> > Add back the combination mode check, but with slightly cleaner code
> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> > The other change is clearer calculations: Just check for zero level
> > explicitly instead of avoiding the divide-by-zero.
> > 
> > Signed-off-by: Indan Zupancic 
> > 
> > ---
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index d860abe..b05631a 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -30,6 +30,10 @@
> > 
> >  #include "intel_drv.h"
> > 
> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> > +#define BLM_COMBINATION_MODE (1 << 30)
> > +#define BLM_LEGACY_MODE (1 << 16)
> > +
> >  void
> >  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >struct drm_display_mode *adjusted_mode)
> > @@ -110,6 +114,22 @@ done:
> > dev_priv->pch_pf_size = (width << 16) | height;
> >  }
> > 
> > +/*
> > + * What about gen 3? If there are no gen 3 systems with ASLE,
> > + * then it doesn't matter, as we don't need to change the
> > + * brightness. But then the gen 2 check can be removed too.
> > + */
> > +static int is_backlight_combination_mode(struct drm_device *dev)
> > +{
> > +   struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +   if (INTEL_INFO(dev)->gen >= 4)
> > +   return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> > +   if (IS_GEN2(dev))
> > +   return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> > +   return 0;
> > +}
> > +
> >  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >  {
> > u32 val;
> > @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
> > *dev)
> > max >>= 17;
> > } else {
> > max >>= 16;
> > +   /* Ignore BLM_LEGACY_MODE bit */
> > if (INTEL_INFO(dev)->gen < 4)
> > max &= ~1;
> > }
> > +   if (is_backlight_combination_mode(dev))
> > +   max *= 0xff;
> > }
> > 
> > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> > @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> > if (IS_PINEVIEW(dev))
> > val >>= 1;
> > +   if (is_backlight_combination_mode(dev)){
> > +   u8 lbpc;
> > +
> > +   pci_read_config_byte(dev->pdev, PCI_LBPC, );
> > +   val *= lbpc;
> > +   }
> > }
> > 
> >

[PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
Indan Zupancic wrote:
> 
> Hello,
> 
> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> > Alex, can you confirm that the revert of 951f3512dba5 plus the
> > one-liner patch from Takashi that Indan quoted also works for you?
> >
> >   Linus
> >
> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic  wrote:
> >>
> >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> most immediate bug without changing anything else. This should go
> >> in stable too.
> >
> 
> I found another backlight bug:
> 
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored.
> 
> This explains the weird behaviour I've seen. I didn't see it with
> combination mode, because then the brightness is always the same
> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> send a patch in a moment.
> 
> Alternative for reverting the combination mode removal (I can also
> redo the patch against the revert and Takashi's patch, if that's
> preferred):
> 
> --
> 
> drm/i915: Do handle backlight combination mode specially
> 
> Add back the combination mode check, but with slightly cleaner code
> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> The other change is clearer calculations: Just check for zero level
> explicitly instead of avoiding the divide-by-zero.
> 
> Signed-off-by: Indan Zupancic 
> 
> ---
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index d860abe..b05631a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -30,6 +30,10 @@
> 
>  #include "intel_drv.h"
> 
> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> +#define BLM_COMBINATION_MODE (1 << 30)
> +#define BLM_LEGACY_MODE (1 << 16)
> +
>  void
>  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>  struct drm_display_mode *adjusted_mode)
> @@ -110,6 +114,22 @@ done:
>   dev_priv->pch_pf_size = (width << 16) | height;
>  }
> 
> +/*
> + * What about gen 3? If there are no gen 3 systems with ASLE,
> + * then it doesn't matter, as we don't need to change the
> + * brightness. But then the gen 2 check can be removed too.
> + */
> +static int is_backlight_combination_mode(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (INTEL_INFO(dev)->gen >= 4)
> + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> + if (IS_GEN2(dev))
> + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> + return 0;
> +}
> +
>  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
>  {
>   u32 val;
> @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
>   max >>= 17;
>   } else {
>   max >>= 16;
> + /* Ignore BLM_LEGACY_MODE bit */
>   if (INTEL_INFO(dev)->gen < 4)
>   max &= ~1;
>   }
> + if (is_backlight_combination_mode(dev))
> + max *= 0xff;
>   }
> 
>   DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
>   val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>   if (IS_PINEVIEW(dev))
>   val >>= 1;
> + if (is_backlight_combination_mode(dev)){
> + u8 lbpc;
> +
> + pci_read_config_byte(dev->pdev, PCI_LBPC, );
> + val *= lbpc;
> + }
>   }
> 
>   DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, 
> u32 level)
> 
>   if (HAS_PCH_SPLIT(dev))
>   return intel_pch_panel_set_backlight(dev, level);
> +
> + if (level && is_backlight_combination_mode(dev)){
> + u32 max = intel_panel_get_max_backlight(dev);
> + u8 lpbc;
> +
> + lpbc = level * 0xff / max;
> + level /= lpbc;

Hmm, I don't think this calculation is correct.  This would result
in level of opregion over its limit.  For example, assume the level
max = 100, so total max = 25500.  Passing level=150 here will be:

lbpc = 150 * 0xff / 25500 = 1.5 = 1
level = 150 / 1 = 150, which is over limit.

More worse, lbpc can be zero when level is below 100 in the case
above...


thanks,

Takashi

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 08:49:37 +0100,
Takashi Iwai wrote:
 
 At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
 Indan Zupancic wrote:
  
  Hello,
  
  On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
   Alex, can you confirm that the revert of 951f3512dba5 plus the
   one-liner patch from Takashi that Indan quoted also works for you?
  
 Linus
  
   On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
  
   So please revert my patch and apply Takashi Iwai's, which fixes the
   most immediate bug without changing anything else. This should go
   in stable too.
  
  
  I found another backlight bug:
  
  When suspending intel_panel_disable_backlight() is never called,
  but intel_panel_enable_backlight() is called at resume. With the
  effect that if the brightness was ever changed after screen
  blanking, the wrong brightness gets restored.
  
  This explains the weird behaviour I've seen. I didn't see it with
  combination mode, because then the brightness is always the same
  (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
  send a patch in a moment.
  
  Alternative for reverting the combination mode removal (I can also
  redo the patch against the revert and Takashi's patch, if that's
  preferred):
  
  --
  
  drm/i915: Do handle backlight combination mode specially
  
  Add back the combination mode check, but with slightly cleaner code
  and the weirdness removed: No val = 1, but also no val = ~1. The
  old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
  The other change is clearer calculations: Just check for zero level
  explicitly instead of avoiding the divide-by-zero.
  
  Signed-off-by: Indan Zupancic in...@nul.nu
  
  ---
  
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index d860abe..b05631a 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -30,6 +30,10 @@
  
   #include intel_drv.h
  
  +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
  +#define BLM_COMBINATION_MODE (1  30)
  +#define BLM_LEGACY_MODE (1  16)
  +
   void
   intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 struct drm_display_mode *adjusted_mode)
  @@ -110,6 +114,22 @@ done:
  dev_priv-pch_pf_size = (width  16) | height;
   }
  
  +/*
  + * What about gen 3? If there are no gen 3 systems with ASLE,
  + * then it doesn't matter, as we don't need to change the
  + * brightness. But then the gen 2 check can be removed too.
  + */
  +static int is_backlight_combination_mode(struct drm_device *dev)
  +{
  +   struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +   if (INTEL_INFO(dev)-gen = 4)
  +   return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
  +   if (IS_GEN2(dev))
  +   return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
  +   return 0;
  +}
  +
   static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
   {
  u32 val;
  @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
  *dev)
  max = 17;
  } else {
  max = 16;
  +   /* Ignore BLM_LEGACY_MODE bit */
  if (INTEL_INFO(dev)-gen  4)
  max = ~1;
  }
  +   if (is_backlight_combination_mode(dev))
  +   max *= 0xff;
  }
  
  DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
  @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
  val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
  if (IS_PINEVIEW(dev))
  val = 1;
  +   if (is_backlight_combination_mode(dev)){
  +   u8 lbpc;
  +
  +   pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
  +   val *= lbpc;
  +   }
  }
  
  DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
  @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, 
  u32 level)
  
  if (HAS_PCH_SPLIT(dev))
  return intel_pch_panel_set_backlight(dev, level);
  +
  +   if (level  is_backlight_combination_mode(dev)){
  +   u32 max = intel_panel_get_max_backlight(dev);
  +   u8 lpbc;
  +
  +   lpbc = level * 0xff / max;
  +   level /= lpbc;
 
 Hmm, I don't think this calculation is correct.  This would result
 in level of opregion over its limit.  For example, assume the level
 max = 100, so total max = 25500.  Passing level=150 here will be:
 
   lbpc = 150 * 0xff / 25500 = 1.5 = 1
   level = 150 / 1 = 150, which is over limit.
 
 More worse, lbpc can be zero when level is below 100 in the case
 above...

That is, Chris' original code in that portion was correct:

if (is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);
u8 lpbc;

lpbc = level * 0xfe / max

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 09:45:18 +0100 (CET),
Indan Zupancic wrote:
 
 On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
  At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
  Indan Zupancic wrote:
 
  Hello,
 
  On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
   Alex, can you confirm that the revert of 951f3512dba5 plus the
   one-liner patch from Takashi that Indan quoted also works for you?
  
 Linus
  
   On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
  
   So please revert my patch and apply Takashi Iwai's, which fixes the
   most immediate bug without changing anything else. This should go
   in stable too.
  
 
  I found another backlight bug:
 
  When suspending intel_panel_disable_backlight() is never called,
  but intel_panel_enable_backlight() is called at resume. With the
  effect that if the brightness was ever changed after screen
  blanking, the wrong brightness gets restored.
 
  This explains the weird behaviour I've seen. I didn't see it with
  combination mode, because then the brightness is always the same
  (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
  send a patch in a moment.
 
  Alternative for reverting the combination mode removal (I can also
  redo the patch against the revert and Takashi's patch, if that's
  preferred):
 
  --
 
  drm/i915: Do handle backlight combination mode specially
 
  Add back the combination mode check, but with slightly cleaner code
  and the weirdness removed: No val = 1, but also no val = ~1. The
  old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
  The other change is clearer calculations: Just check for zero level
  explicitly instead of avoiding the divide-by-zero.
 
  Signed-off-by: Indan Zupancic in...@nul.nu
 
  ---
 
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index d860abe..b05631a 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -30,6 +30,10 @@
 
   #include intel_drv.h
 
  +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
  +#define BLM_COMBINATION_MODE (1  30)
  +#define BLM_LEGACY_MODE (1  16)
  +
   void
   intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode)
  @@ -110,6 +114,22 @@ done:
 dev_priv-pch_pf_size = (width  16) | height;
   }
 
  +/*
  + * What about gen 3? If there are no gen 3 systems with ASLE,
  + * then it doesn't matter, as we don't need to change the
  + * brightness. But then the gen 2 check can be removed too.
  + */
  +static int is_backlight_combination_mode(struct drm_device *dev)
  +{
  +  struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +  if (INTEL_INFO(dev)-gen = 4)
  +  return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
  +  if (IS_GEN2(dev))
  +  return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
  +  return 0;
  +}
  +
   static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
   {
 u32 val;
  @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
  *dev)
 max = 17;
 } else {
 max = 16;
  +  /* Ignore BLM_LEGACY_MODE bit */
 if (INTEL_INFO(dev)-gen  4)
 max = ~1;
 }
  +  if (is_backlight_combination_mode(dev))
  +  max *= 0xff;
 }
 
 DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
  @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
 if (IS_PINEVIEW(dev))
 val = 1;
  +  if (is_backlight_combination_mode(dev)){
  +  u8 lbpc;
  +
  +  pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
  +  val *= lbpc;
  +  }
 }
 
 DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
  @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device 
  *dev, u32 level)
 
 if (HAS_PCH_SPLIT(dev))
 return intel_pch_panel_set_backlight(dev, level);
  +
  +  if (level  is_backlight_combination_mode(dev)){
  +  u32 max = intel_panel_get_max_backlight(dev);
  +  u8 lpbc;
  +
  +  lpbc = level * 0xff / max;
  +  level /= lpbc;
 
  Hmm, I don't think this calculation is correct.  This would result
  in level of opregion over its limit.  For example, assume the level
  max = 100, so total max = 25500.  Passing level=150 here will be:
 
  lbpc = 150 * 0xff / 25500 = 1.5 = 1
  level = 150 / 1 = 150, which is over limit.
 
  More worse, lbpc can be zero when level is below 100 in the case
  above...
 
 Yes, you're right. It seems that any simplification I try to do
 creates a new bug.
 
 Do you have any bright idea why the old code did val = ~1; too?
 It seems obvious it's related to val = 1, but...

I guess it's for the case

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Takashi Iwai]

At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
 
 On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
  At Thu, 10 Mar 2011 08:49:37 +0100,
  Takashi Iwai wrote:
 
  At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
  Indan Zupancic wrote:
  
   Hello,
  
   On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the
one-liner patch from Takashi that Indan quoted also works for you?
   
  Linus
   
On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
   
So please revert my patch and apply Takashi Iwai's, which fixes the
most immediate bug without changing anything else. This should go
in stable too.
   
  
   I found another backlight bug:
  
   When suspending intel_panel_disable_backlight() is never called,
   but intel_panel_enable_backlight() is called at resume. With the
   effect that if the brightness was ever changed after screen
   blanking, the wrong brightness gets restored.
  
   This explains the weird behaviour I've seen. I didn't see it with
   combination mode, because then the brightness is always the same
   (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
   send a patch in a moment.
  
   Alternative for reverting the combination mode removal (I can also
   redo the patch against the revert and Takashi's patch, if that's
   preferred):
  
   --
  
   drm/i915: Do handle backlight combination mode specially
  
   Add back the combination mode check, but with slightly cleaner code
   and the weirdness removed: No val = 1, but also no val = ~1. The
   old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
   The other change is clearer calculations: Just check for zero level
   explicitly instead of avoiding the divide-by-zero.
  
   Signed-off-by: Indan Zupancic in...@nul.nu
  
   ---
  
   diff --git a/drivers/gpu/drm/i915/intel_panel.c 
   b/drivers/gpu/drm/i915/intel_panel.c
   index d860abe..b05631a 100644
   --- a/drivers/gpu/drm/i915/intel_panel.c
   +++ b/drivers/gpu/drm/i915/intel_panel.c
   @@ -30,6 +30,10 @@
  
#include intel_drv.h
  
   +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
   +#define BLM_COMBINATION_MODE (1  30)
   +#define BLM_LEGACY_MODE (1  16)
   +
void
intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
   struct drm_display_mode *adjusted_mode)
   @@ -110,6 +114,22 @@ done:
dev_priv-pch_pf_size = (width  16) | height;
}
  
   +/*
   + * What about gen 3? If there are no gen 3 systems with ASLE,
   + * then it doesn't matter, as we don't need to change the
   + * brightness. But then the gen 2 check can be removed too.
   + */
   +static int is_backlight_combination_mode(struct drm_device *dev)
   +{
   +struct drm_i915_private *dev_priv = dev-dev_private;
   +
   +if (INTEL_INFO(dev)-gen = 4)
   +return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
   +if (IS_GEN2(dev))
   +return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
   +return 0;
   +}
   +
static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
{
u32 val;
   @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
   *dev)
max = 17;
} else {
max = 16;
   +/* Ignore BLM_LEGACY_MODE bit */
if (INTEL_INFO(dev)-gen  4)
max = ~1;
}
   +if (is_backlight_combination_mode(dev))
   +max *= 0xff;
}
  
DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
   @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device 
   *dev)
val = I915_READ(BLC_PWM_CTL)  
   BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val = 1;
   +if (is_backlight_combination_mode(dev)){
   +u8 lbpc;
   +
   +pci_read_config_byte(dev-pdev, PCI_LBPC, 
   lbpc);
   +val *= lbpc;
   +}
}
  
DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
   @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device 
   *dev, u32 level)
  
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
   +
   +if (level  is_backlight_combination_mode(dev)){
   +u32 max = intel_panel_get_max_backlight(dev);
   +u8 lpbc;
   +
   +lpbc = level * 0xff / max;
   +level /= lpbc;
 
  Hmm, I don't think this calculation is correct.  This would result
  in level of opregion over its limit.  For example, assume the level
  max = 100, so total max = 25500.  Passing level

[PATCH] drm/i915: Revive combination mode for backlight control

[Takashi Iwai]

This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb

drm/i915: Do not handle backlight combination mode specially

since this commit introduced other regressions due to untouched LBPC
register, e.g. the backlight dimmed after resume.

In addition to the revert, this patch includes a fix for the original
issue (weird backlight levels) by removing the wrong bit shift for
computing the current backlight level.
Also, including typo fixes (lpbc - lbpc).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
Acked-by: Indan Zupancic in...@nul.nu
Cc: sta...@kernel.org
Signed-off-by: Takashi Iwai ti...@suse.de
---
 drivers/gpu/drm/i915/i915_reg.h|   10 ++
 drivers/gpu/drm/i915/intel_panel.c |   36 
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3e6f486..2abe240 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1553,7 +1553,17 @@
 
 /* Backlight control */
 #define BLC_PWM_CTL0x61254
+#define   BACKLIGHT_MODULATION_FREQ_SHIFT  (17)
 #define BLC_PWM_CTL2   0x61250 /* 965+ only */
+#define   BLM_COMBINATION_MODE (1  30)
+/*
+ * This is the most significant 15 bits of the number of backlight cycles in a
+ * complete cycle of the modulated backlight control.
+ *
+ * The actual value is this field multiplied by two.
+ */
+#define   BACKLIGHT_MODULATION_FREQ_MASK   (0x7fff  17)
+#define   BLM_LEGACY_MODE  (1  16)
 /*
  * This is the number of cycles out of the backlight modulation cycle for which
  * the backlight is on.
diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..f8f86e5 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,6 +30,8 @@
 
 #include intel_drv.h
 
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
+
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
   struct drm_display_mode *adjusted_mode)
@@ -110,6 +112,19 @@ done:
dev_priv-pch_pf_size = (width  16) | height;
 }
 
+static int is_backlight_combination_mode(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev-dev_private;
+
+   if (INTEL_INFO(dev)-gen = 4)
+   return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
+
+   if (IS_GEN2(dev))
+   return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
+
+   return 0;
+}
+
 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
 {
u32 val;
@@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
if (INTEL_INFO(dev)-gen  4)
max = ~1;
}
+
+   if (is_backlight_combination_mode(dev))
+   max *= 0xff;
}
 
DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
@@ -183,6 +201,14 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val = 1;
+
+   if (is_backlight_combination_mode(dev)){
+   u8 lbpc;
+
+   val = ~1;
+   pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
+   val *= lbpc;
+   }
}
 
DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
@@ -205,6 +231,16 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 
level)
 
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
+
+   if (is_backlight_combination_mode(dev)){
+   u32 max = intel_panel_get_max_backlight(dev);
+   u8 lbpc;
+
+   lbpc = level * 0xfe / max + 1;
+   level /= lbpc;
+   pci_write_config_byte(dev-pdev, PCI_LBPC, lbpc);
+   }
+
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
-- 
1.7.4.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Takashi Iwai]

[Removed stable-kernel from Cc]

At Fri, 11 Mar 2011 02:35:45 +0100 (CET),
Indan Zupancic wrote:
 
 drm/i915: Fix DPMS and suspend interaction for intel_panel.c
 
 When suspending intel_panel_disable_backlight() is never called,
 but intel_panel_enable_backlight() is called at resume. With the
 effect that if the brightness was ever changed after screen
 blanking, the wrong brightness gets restored at resume time.
 
 Nothing guarantees that those calls will be balanced, so having
 backlight_enabled makes no sense, as the real state can change
 without the panel code noticing. So keep things as stateless as
 possible.
 
 Signed-off-by: Indan Zupancic in...@nul.nu

Indan, when you need a patch to be added to stable kernel, just put
Cc to stable kernel around your sign-off line.  Then Greg will pick it
up automatically once when the patch is merged to Linus tree.

Anyway, the patch looks good to me.  A nice clean-up.

Reviewed-by: Takashi Iwai ti...@suse.de


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2.6.38-rc3-git1: Reported regressions 2.6.36 -> 2.6.37

[Takashi Iwai]

At Thu, 3 Feb 2011 17:11:14 -0800,
Linus Torvalds wrote:
> 
> On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard  wrote:
> >
> > The goal is to make it so that when you *do* set a mode, DPMS gets set
> > to ON (as the monitor will actually be "on" at that point). Here's a
> > patch which does the DPMS_ON precisely when setting a mode.
> 
> Ok, patch looks sane, but it does leave me with the "what about the
> 'fb_changed' case?" question. Is that case basically guaranteed to not
> change any existing dpms state?
> 
> > (note, this patch compiles, but is otherwise only lightly tested).
> 
> Carlos? Takashi? Ignore my crazy patch, try this one instead. Does it
> fix things for you?

Yes, the patch fixes the issue with xrandr off and on.

However, another issue I reported in that bugzilla still remains:
namely, DPMS value returned via ioctl or obtained via sysfs is
inconsistent with the actually applied value.   The reason is that
there are two places keeping the current DPMS values, in connector and
in crtc device properties.  A similar fix like my patch in the
bugzilla would be still needed, I guess.


thanks,

Takashi

Re: 2.6.38-rc3-git1: Reported regressions 2.6.36 - 2.6.37

[Takashi Iwai]

At Thu, 3 Feb 2011 17:11:14 -0800,
Linus Torvalds wrote:
 
 On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard kei...@keithp.com wrote:
 
  The goal is to make it so that when you *do* set a mode, DPMS gets set
  to ON (as the monitor will actually be on at that point). Here's a
  patch which does the DPMS_ON precisely when setting a mode.
 
 Ok, patch looks sane, but it does leave me with the what about the
 'fb_changed' case? question. Is that case basically guaranteed to not
 change any existing dpms state?
 
  (note, this patch compiles, but is otherwise only lightly tested).
 
 Carlos? Takashi? Ignore my crazy patch, try this one instead. Does it
 fix things for you?

Yes, the patch fixes the issue with xrandr off and on.

However, another issue I reported in that bugzilla still remains:
namely, DPMS value returned via ioctl or obtained via sysfs is
inconsistent with the actually applied value.   The reason is that
there are two places keeping the current DPMS values, in connector and
in crtc device properties.  A similar fix like my patch in the
bugzilla would be still needed, I guess.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2.6.38-rc3-git1: Reported regressions 2.6.36 -> 2.6.37

[Takashi Iwai]

At Thu, 3 Feb 2011 07:42:05 -0800,
Linus Torvalds wrote:
> 
> On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra  wrote:
> > On Thu ?3.Feb'11 at ?1:03:41 +0100, Rafael J. Wysocki wrote:
> >>
> >> If you know of any other unresolved post-2.6.36 regressions, please let us 
> >> know
> >> either and we'll add them to the list. ?Also, please let us know if any
> >> of the entries below are invalid.
> >
> > I'm sorry if I'm overlooking something, but as far as I can see the 
> > regression
> > reported here:
> >
> > https://lkml.org/lkml/2011/1/24/457
> >
> > is not in the list (update on that report: reverting that commit on top of
> > 2.6.37 fixes the issue).
> 
> Ok, added Keith and Dave to the cc, since they are the signers of that commit.
> 
> > After some time, I also ended up finding an earlier report in the kernel 
> > bugzilla
> > which I think is the same regression (it was bisected to the same commit):
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=24982
> >
> > but I do not see it in the list either, even though it's marked as a
> > regression in the bugzilla.
> >
> > The issue was also present in 2.6.38-rc2 last time I tested.
> 
> Just to confirm, can you also check -rc3? I'm pretty sure nothing has
> changed, but there were a few drm patches after -rc2, so it's alsways
> good to double-check.

The problem I reported in the bugzilla above is still present in
2.6.38-rc3.  I'm pretty sure that's the same issue as Carlos' case.


thanks,

Takashi

Re: 2.6.38-rc3-git1: Reported regressions 2.6.36 - 2.6.37

[Takashi Iwai]

At Thu, 3 Feb 2011 07:42:05 -0800,
Linus Torvalds wrote:
 
 On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra crmaf...@gmail.com wrote:
  On Thu  3.Feb'11 at  1:03:41 +0100, Rafael J. Wysocki wrote:
 
  If you know of any other unresolved post-2.6.36 regressions, please let us 
  know
  either and we'll add them to the list.  Also, please let us know if any
  of the entries below are invalid.
 
  I'm sorry if I'm overlooking something, but as far as I can see the 
  regression
  reported here:
 
  https://lkml.org/lkml/2011/1/24/457
 
  is not in the list (update on that report: reverting that commit on top of
  2.6.37 fixes the issue).
 
 Ok, added Keith and Dave to the cc, since they are the signers of that commit.
 
  After some time, I also ended up finding an earlier report in the kernel 
  bugzilla
  which I think is the same regression (it was bisected to the same commit):
 
  https://bugzilla.kernel.org/show_bug.cgi?id=24982
 
  but I do not see it in the list either, even though it's marked as a
  regression in the bugzilla.
 
  The issue was also present in 2.6.38-rc2 last time I tested.
 
 Just to confirm, can you also check -rc3? I'm pretty sure nothing has
 changed, but there were a few drm patches after -rc2, so it's alsways
 good to double-check.

The problem I reported in the bugzilla above is still present in
2.6.38-rc3.  I'm pretty sure that's the same issue as Carlos' case.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Add a reset interface

[Takashi Iwai]

At Mon, 24 Jan 2011 15:55:27 +,
Chris Wilson wrote:
 
 For i915 there is a need to invalidate some cached state after resuming or
 reseting the GPU. This is not quite the same as simply restoring saved
 state (i.e. the standard suspend resume method), so do not seem to merit
 reusing the save|restore vfuncs.  Instead I propose a
 
drm_mode_config_reset(struct drm_device *);
 
 routine to iterate over all the attached CRTCs, encoders and connectors
 and call any supplied reset vfunc.
 
 This is required to fix some modesetting regressions across resume in
 2.6.38:
 https://bugzilla.kernel.org/show_bug.cgi?id=26952
 https://bugzilla.kernel.org/show_bug.cgi?id=27272

I quickly tried these patches.  After adding the missing EXPORT_SYMBOL,
it seems working fine.  Tested on a SNB laptop and a PineView laptop.

Put my tag to all patches:
  Tested-by: Takashi Iwai ti...@suse.de
  

Thanks!

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[git pull] drm fixes

[Takashi Iwai]

At Wed, 22 Dec 2010 16:59:06 +,
Chris Wilson wrote:
> 
> On Wed, 22 Dec 2010 17:24:36 +0100, Takashi Iwai  wrote:
> > The commit 448f53a1ede54eb854d036abf54573281412d650
> >   drm/i915/bios: Reverse order of 100/120 Mhz SSC clocks
> > 
> > causes a regression on a SandyBridge machine here.
> > The laptop display (LVDS) becomes blank.  Reverting the commit fixes
> > the problem.
> 
> The question is whose BIOS is wrong? The Lenovo U160's or the
> Sandybridge SDV? And why does it work for that other OS?  rhetorical question of the day here.>
> 
> It's back to the square one for one or the other platform...

Yeah, we can blame BIOS :)  And, this is likely the BIOS on my machine
here that is broken.

But this seems like an issue that you can't rely solely on VBT.  We
can never guarantee that BIOS is correct (who can?), and there is no
way to avoid this change as long as it's hard-coded.  We've hit
another regression by VBT check (e-DP  wrongly detected; kernel bug
24822), so I think judging the behavior only from BIOS is rather
dangerous.


thanks,

Takashi

[git pull] drm fixes

[Takashi Iwai]

At Tue, 21 Dec 2010 23:43:18 + (GMT),
Dave Airlie wrote:
> 
> 
> Hi,
> 
> meant to get this out earlier, but I've been off sick as well as having a 
> sick kid, also meant a few things piled up when I wasn't looking
> 
> contains a revert for reported regression in intel and also one in radeon,
> a few radeon fixes, one for a 15s resume time on certain laptops, and one 
> to fix gpu reset on some gpus,
> 
> Dave.
> 
> The following changes since commit a4851d8f7d6351a395d36ae8fdcf41745a832d76:
> 
>   Merge branch 'for_linus' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4 (2010-12-15 12:41:17 
> -0800)
> 
> are available in the git repository at:
> 
>   ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git 
> drm-fixes
> 
> Alex Deucher (7):
>   drm/radeon/kms: disable ss fixed ref divide
>   drm/radeon/kms: disable the r600 cb offset checker for linear surfaces
>   drm/radeon/kms/evergreen: flush hdp cache when flushing gart tlb
>   drm/radeon/kms: fix evergreen asic reset
>   drm/radeon/kms/evergreen: reset the grbm blocks at resume and init
>   drm/radeon/kms: reorder display resume to avoid problems
>   drm/radeon/kms: fix bug in r600_gpu_is_lockup
> 
> Benjamin Herrenschmidt (1):
>   drm/radeon: Add early unregister of firmware fb's
> 
> Chris Wilson (5):
>   drm/i915/ringbuffer: Handle wrapping of the autoreported HEAD
>   drm/i915/sdvo: Only use the SDVO pin if it is in the valid range
>   agp/intel: Fix missed cached memory flags setting in i965_write_entry()
>   drm/i915/bios: Reverse order of 100/120 Mhz SSC clocks
>   drm: Include the connector name in the output_poll_execute() debug 
> message

The commit 448f53a1ede54eb854d036abf54573281412d650
  drm/i915/bios: Reverse order of 100/120 Mhz SSC clocks

causes a regression on a SandyBridge machine here.
The laptop display (LVDS) becomes blank.  Reverting the commit fixes
the problem.


thanks,

Takashi

Re: [git pull] drm fixes

[Takashi Iwai]

At Wed, 22 Dec 2010 16:59:06 +,
Chris Wilson wrote:
 
 On Wed, 22 Dec 2010 17:24:36 +0100, Takashi Iwai ti...@suse.de wrote:
  The commit 448f53a1ede54eb854d036abf54573281412d650
drm/i915/bios: Reverse order of 100/120 Mhz SSC clocks
  
  causes a regression on a SandyBridge machine here.
  The laptop display (LVDS) becomes blank.  Reverting the commit fixes
  the problem.
 
 The question is whose BIOS is wrong? The Lenovo U160's or the
 Sandybridge SDV? And why does it work for that other OS? Insert
 rhetorical question of the day here.
 
 It's back to the square one for one or the other platform...

Yeah, we can blame BIOS :)  And, this is likely the BIOS on my machine
here that is broken.

But this seems like an issue that you can't rely solely on VBT.  We
can never guarantee that BIOS is correct (who can?), and there is no
way to avoid this change as long as it's hard-coded.  We've hit
another regression by VBT check (e-DP  wrongly detected; kernel bug
24822), so I think judging the behavior only from BIOS is rather
dangerous.


thanks,

Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel