diff mbox

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

Message ID 1393060.SF8x67qbTg@avalon (mailing list archive)
State Superseded
Headers show

Commit Message

Laurent Pinchart June 14, 2013, 12:54 a.m. UTC
Hi Daniel,

On Friday 07 June 2013 10:50:55 Daniel Vetter wrote:
> On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
> > 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.
> 
> We might want to add such a default mode also for unknown, I'm not sure.
> Userspace policy is to first try to light up any connected outputs, and if
> there's none try to light up any unknown outputs. Not sure whether userspace
> (i.e. X) will automatically add a default mode. fbcon might also handle this
> less gracefully.
> 
> Personally I'm ok with extending this to unknown, it shouldn't really hurt
> (since we already try really hard not to leak unknown anywhere visible).

Do you mean something like


If so I can submit a proper patch.

> > - 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.
> 
> Atm you can set that with the kernel video= cmdline option, but only if
> fbcon force this to be parsed. I think exposing ->force to userspace
> somewhere in sysfs would make lots of sense. Drivers imo shouldn't ever
> need to touch this. And there's a callback interface so that drivers can
> intercept forced connector state, e.g. when they need to set up some stuff
> which they otherwise would only do in their ->detect callback.
> 
> > > 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..
> 
> There's also a kernel option. Since we're talking about a VGA connector I
> don't think we could do a hardwired quirk here.
> 
> > > - 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 ?
> 
> Changed my mind ;-) Ajax recently said that X only looks at unknown
> connectors if there's nothing better around, so if we stick to that policy
> we should be good.
> 
> > > 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 ;-)

Comments

Daniel Vetter June 14, 2013, 2:03 p.m. UTC | #1
On Fri, Jun 14, 2013 at 02:54:04AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Friday 07 June 2013 10:50:55 Daniel Vetter wrote:
> > On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
> > > 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.
> > 
> > We might want to add such a default mode also for unknown, I'm not sure.
> > Userspace policy is to first try to light up any connected outputs, and if
> > there's none try to light up any unknown outputs. Not sure whether userspace
> > (i.e. X) will automatically add a default mode. fbcon might also handle this
> > less gracefully.
> > 
> > Personally I'm ok with extending this to unknown, it shouldn't really hurt
> > (since we already try really hard not to leak unknown anywhere visible).
> 
> Do you mean something like
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index f554516..9aae384 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -160,7 +160,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  #endif
>  		count = (*connector_funcs->get_modes)(connector);
>  
> -	if (count == 0 && connector->status == connector_status_connected)
> +	if (count == 0 && (connector->status == connector_status_connected ||
> +			   connector->status == connector_status_unknown))
>  		count = drm_add_modes_noedid(connector, 1024, 768);
>  	if (count == 0)
>  		goto prune;
> 
> If so I can submit a proper patch.

Yeah, but Ajax is the guy with the opinion that matters here. Cc'ed.
-Daniel
Laurent Pinchart June 18, 2013, 11:45 p.m. UTC | #2
Hello Adam,

Ping ?

Daniel, would it help getting the driver in v3.11 if I resubmit it now with a 
get_modes operation that just returns 0 ?

On Friday 14 June 2013 16:03:19 Daniel Vetter wrote:
> On Fri, Jun 14, 2013 at 02:54:04AM +0200, Laurent Pinchart wrote:
> > On Friday 07 June 2013 10:50:55 Daniel Vetter wrote:
> > > On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
> > > > 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.
> > > 
> > > We might want to add such a default mode also for unknown, I'm not sure.
> > > Userspace policy is to first try to light up any connected outputs, and
> > > if> there's none try to light up any unknown outputs. Not sure whether
> > > userspace (i.e. X) will automatically add a default mode. fbcon might
> > > also handle this less gracefully.
> > > 
> > > Personally I'm ok with extending this to unknown, it shouldn't really
> > > hurt (since we already try really hard not to leak unknown anywhere
> > > visible).
> > 
> > Do you mean something like
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> > b/drivers/gpu/drm/drm_crtc_helper.c index f554516..9aae384 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -160,7 +160,8 @@ int drm_helper_probe_single_connector_modes(struct
> > drm_connector *connector,> 
> >  #endif
> >  		count = (*connector_funcs->get_modes)(connector);
> > -	if (count == 0 && connector->status == connector_status_connected)
> > +	if (count == 0 && (connector->status == connector_status_connected ||
> > +			   connector->status == connector_status_unknown))
> >  		count = drm_add_modes_noedid(connector, 1024, 768);
> >  	if (count == 0)
> >  		goto prune;
> > 
> > If so I can submit a proper patch.
> 
> Yeah, but Ajax is the guy with the opinion that matters here. Cc'ed.
Daniel Vetter June 20, 2013, 2:06 p.m. UTC | #3
On Wed, Jun 19, 2013 at 01:45:45AM +0200, Laurent Pinchart wrote:
> Hello Adam,
> 
> Ping ?
> 
> Daniel, would it help getting the driver in v3.11 if I resubmit it now with a 
> get_modes operation that just returns 0 ?

Yeah, I guess that works, too.
-Daniel

> 
> On Friday 14 June 2013 16:03:19 Daniel Vetter wrote:
> > On Fri, Jun 14, 2013 at 02:54:04AM +0200, Laurent Pinchart wrote:
> > > On Friday 07 June 2013 10:50:55 Daniel Vetter wrote:
> > > > On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
> > > > > 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.
> > > > 
> > > > We might want to add such a default mode also for unknown, I'm not sure.
> > > > Userspace policy is to first try to light up any connected outputs, and
> > > > if> there's none try to light up any unknown outputs. Not sure whether
> > > > userspace (i.e. X) will automatically add a default mode. fbcon might
> > > > also handle this less gracefully.
> > > > 
> > > > Personally I'm ok with extending this to unknown, it shouldn't really
> > > > hurt (since we already try really hard not to leak unknown anywhere
> > > > visible).
> > > 
> > > Do you mean something like
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> > > b/drivers/gpu/drm/drm_crtc_helper.c index f554516..9aae384 100644
> > > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > > @@ -160,7 +160,8 @@ int drm_helper_probe_single_connector_modes(struct
> > > drm_connector *connector,> 
> > >  #endif
> > >  		count = (*connector_funcs->get_modes)(connector);
> > > -	if (count == 0 && connector->status == connector_status_connected)
> > > +	if (count == 0 && (connector->status == connector_status_connected ||
> > > +			   connector->status == connector_status_unknown))
> > >  		count = drm_add_modes_noedid(connector, 1024, 768);
> > >  	if (count == 0)
> > >  		goto prune;
> > > 
> > > If so I can submit a proper patch.
> > 
> > Yeah, but Ajax is the guy with the opinion that matters here. Cc'ed.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index f554516..9aae384 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -160,7 +160,8 @@  int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 #endif
 		count = (*connector_funcs->get_modes)(connector);
 
-	if (count == 0 && connector->status == connector_status_connected)
+	if (count == 0 && (connector->status == connector_status_connected ||
+			   connector->status == connector_status_unknown))
 		count = drm_add_modes_noedid(connector, 1024, 768);
 	if (count == 0)
 		goto prune;