Message-ID: <1285023750.30856.99.camel@morgan.silverblock.net>
Date:	Mon, 20 Sep 2010 19:02:30 -0400
From:	Andy Walls <awalls@...metrocast.net>
To:	Rafał Miłecki <zajec5@...il.com>
Cc:	linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
	Jon Smirl <jonsmirl@...il.com>,
	dri-devel@...ts.freedesktop.org,
	Greg Kroah-Hartman <greg@...ah.com>
Subject: Re: [PATCH] drm/sysfs: Provide per connector control of DRM KMS
 polling

On Mon, 2010-09-20 at 20:29 +0200, Rafał Miłecki wrote:
> 2010/9/20 Andy Walls <awalls@...metrocast.net>:
> > DRM KMS polling of connections providing errant EDID responses, or
> > polling of "connectors" that have chips responding on DDC I2C bus
> > address 0xA0/0xA1 with no actual physical connector nor EDID EEPROM,
> > will create perpetual noise in dmesg and the system log every 10
> > seconds.  Currently the user has apparently little recourse to silence
> > these messages aside from replacing the offending cable, monitor, or
> > graphics adapter.  That recourse is impossible for an unused DVI-D
> > "connector" of an internal graphics processor on a motherboard that
> > provides no physical DVI-D connector.
> >
> > This change allows the root user to disable (and re-enable) DRM KMS
> > connector polling on a per connector basis via sysfs, like so:
> 
> Huh, I didn't imagine solution of this issue that way.

The problems with my initial patch, and related approaches, were:

1. They didn't get rid of all the spam in the logs, just the most
verbose portion of it.

2. They didn't deal with the root cause of the log spam and all the
related, but unneeded, I/O and processing that was still occurring.



>  AFAIR previous
> thread, something else was suggested, it was about storing the fact
> that user already received warning/info about error.

Yes, that was the discussion.

I went with this patch because:

1. It deals with the root cause: unneeded DRM KMS connector polling

2. It eliminates all my DRM KMS connector polling related log spam, not
just the hex dump.

3. The existing sysfs structure was already exposing 'struct
drm_connector' members to user-space at per connector granularity


The real root cause of the polling spew is that the radeon module, based
on the best information it has (BIOS connector tables, I2C bus probes,
etc.), makes a decision on if a connector should be polled and how it
should be polled.

Given incorrect BIOS tables, and strange I2C setups, the radeon module
is bound to make the wrong decision about polling in some cases.  This
change is a way for the human to step in a correct things, when the
radeon driver gets it wrong.

In my case, along with the log spam, there are also a lot of unneeded
I2C transactions going on.


BTW, I found that Chris Wilson recently committed a change to inhibit
all drm connector polling globally for a different reason:

http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=commit;h=e58f637bb96d5a0ae0919b9998b891d1ba7e47c9

That commit message shows a case where the driver decides polling needs
to happen, but the human knows differently and manual control over
connector polling mitigates the problem.





> Why I don't really like proposed solution?
> 1) You need to be root

Yes, for both sysfs and module parameters.

I view video hardware configuration (montiors, cables, graphics cards)
and control of what gets logged into files as system administrative
issues.  I'd assume that needing root privileges to turn on and off
device auto detection would be the norm.  I imagined that root would set
up any DRM KMS connector polling fixes once in an init script and be
done with it.  Is there any other use-case you had in mind?

If there's some set-uid or group privilege method or utilities that's in
common use (xrandr?) for manual control of drm/kms parameters, that you
think would be better, just point me in that direction and I'll try to
work something out.

However, trying to code automatic control of when to disable DRM KMS
connector polling, by some heuristic or algorithm in radeon or drm, is
likely doomed to failure for corner cases or for someone's policy
preferences.  I'd rather not attempt that.


> 2) Need to know and play with some "magic" sysfs file.

I strongly dislike ever using sysfs myself.  However, I'll spare us all
my rantings^Wreasons. ;)

My problem with KMS was with the documentation I could find.  The
"userland interfaces" section in Documentation/Docbook/drm.tmpl is
essentially empty.  DRM design documents at freedesktop.org (which
actually look closer to requirements documents) are the most detailed
documents I could find, but they don't really cover KMS.  Maybe I'm just
looking in the wrong places.

Is there a KMS or DRM_MODESET API document?  Maybe I could propose a new
ioctl(), if there isn't already one that will be usable for controlling
DRM KMS polling.



> I thing it would be much better and user-friendly to display such an
> error just once per display. How this could be implemented?
> 
> 1) Add field like "warned" to connector struct, default to 0
> 2) if (warning_to_be_printed && !connector->warned) { printk();
> connector->warned = 1
> 3) if (connector_disconnected) { connector->warned = 0; }
> 
> This was you will get just one warning per connected display. Of
> course after disconnecting it we have to clear "warned", as user may
> connect another "broken" display, then we should print warning again.
> 
> What do think about this solution?

I followed most of the function calls back and looked at he return
values and error messages that can get emitted under various
circumstances.  The amount of effort and complexity for trying to
supress all the error messages due to the 10 second poll just wasn't
worth it, when supressing the polling was so much easier.  For
supressing error messages, I decided there was too much state to track
and that any policy I hard-coded would be wrong for someone.

>>From inside the code, it is difficult to decide what message about
errors induced from the external environment are important to the user
and what aren't.  It's too dependent on the external context and what
action the user would like to take under various conditions.


Thanks for your comments.  Sorry for the long reply.

Regards,
Andy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists