Re: [PATCH v3] drm: Renesas R-Car Display Unit DRM driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ;-)

-- 
Regards,

Laurent Pinchart