Hi Daniel,
On Wednesday 05 June 2013 10:55:05 Daniel Vetter wrote:
On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote:
On Tuesday 04 June 2013 20:36:20 Daniel Vetter wrote:
On Tue, Jun 4, 2013 at 8:03 PM, Laurent Pinchart wrote:
On Tuesday 04 June 2013 16:12:36 Daniel Vetter wrote:
On Tue, Jun 04, 2013 at 04:53:40AM +0200, Laurent Pinchart wrote:
[snip]
+static int rcar_du_vga_connector_get_modes(struct drm_connector *connector) +{
- return drm_add_modes_noedid(connector, 1280, 768);
+}
This (and the dummy detect function below) looks a bit funny, since it essentially overrides the default behaviour already provided by the crtc helpers. Until rcar has at least proper detect support for VGA
I would add that but the DDC signals are not connected on the boards I have access to :-/
I'd just kill this and use the connector force support (and manually adding the right resolutions).
Looks like that's a candidate for better documentation... How does force support work ?
Grep for DRM_FORCE_ON, iirc it can be set on the commandline, where you can also force a specific mode. The best I could find wrt docs is the kerneldoc for drm_mode_parse_command_line_for_connector. With a bit more reading it looks like it's intermingled with the fbdev helper code, but should be fairly easy to extract and used by your driver.
It makes sense to force the connector state from command line, but I'm not sure if the same mechanism is the best solution here. As the driver has no way to know the connector state, the best we can do is guess what modes are supported. I can just return 0 in the get_modes handler, but then the core will not call drm_add_modes_noedid(), and modes will need to be added manually.
Is your point that for a board on which the VGA connector state can't be detected, the user should always be responsible for adding all the modes supported by the VGA monitor on the command line ?
My point is that we already have both an established code for connected outputs without EDID to add fallback modes and means to force connectors to certain states. Your code here seems to reinvent that wheel, so I wonder what we should/need to improve in the common code to suit your needs.
The currently available code might suit my needs, it might just be that I fail to see how to use it properly.
Regarding the "code for connected outputs without EDID to add fallback modes" you're referring to, is that
if (count == 0 && connector->status == connector_status_connected) count = drm_add_modes_noedid(connector, 1024, 768);
in drm_helper_probe_single_connector_modes() ? That function will only be called if the connector status is connector_status_connected. There are two ways to enforce that:
- returning connector_status_connected from the connector detect() operation, which seems to defeat the purpose of having connector_status_unknown completely.
- setting connector->force to DRM_FORCE_ON. Are drivers allowed to do so themselves at initialization time ? Once again that seems to defeat the purpose of connector_status_unknown.
A few ideas:
- Untangling the connector forcing code from the fbdev helper so that you can use it.
- Exposing the connector state forcing through sysfs so that it's runtime-adjustable.
My main concern here is that fbcon won't be available if we delay setting force mode until userspace is ready..
- Adding fallback modes for connectors in the unknonw state (imo too much risk in breaking something else).
Could you please elaborate on what you thing it could break ?
Thinking about this some more I'd vote for the new sysfs file to expose connector forcing at runtime. With that it'd boil down to 1024x756 vs. 1280x768 for the default fallback mode. And that could be fixed with the EDID quirk support. Although that looks like it would benefit from a per-connector sysfs file, too ;-)