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

[Daniel Vetter]

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 

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

[Daniel Vetter]

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

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

[Laurent Pinchart]

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 

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

[Laurent Pinchart]

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 

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

[Daniel Vetter]

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

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

[Laurent Pinchart]

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.

> > - setting connector->force to DRM_FORCE_ON. Are drivers allowed to do so
> > themselves at 

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list

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

[Laurent Pinchart]

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.

  - 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 

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

[Daniel Vetter]

On Fri, Jun 07, 2013 at 09:23:58AM +0200, Laurent Pinchart wrote:
> On Thursday 06 June 2013 09:21:35 Alex Deucher wrote:
> > On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter wrote:
> > > On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher wrote:
> > >> To me at least, it doesn't make sense that an encoder can clone
> > >> itself.  If an encoder is already in use, trying to clone itself would
> > >> only lead to confusion and possible bugs (make sure some code path
> > >> doesn't try and reprogram the encoder again, etc.).
> > > 
> > > For me the possible_clones mask is just the set of encoders which can
> > > together share a crtc (presuming that crtc is indeed in all of the
> > > possible_crtcs mask of each encoder). From that pov it makes imo sense
> > > that a given encoder itself can always be with itself on the same crtc
> > > ;-)
> > > 
> > > Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
> > > internally use them, but it's not interface). And the possible_clones
> > > stuff is by far not enough to describe all hw restrictions. So tbh I
> > > don't care which way we go (or whether we indeed keep on using this
> > > much at all).
> > 
> > Same.  I can go either way.
> 
> So what's the agreement ? :-)

I think officially "meh". Keep your current code and we'll try to fix this
once it blows up somewhere for real ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
> 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.

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

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

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

[Laurent Pinchart]

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

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

[Laurent Pinchart]

On Thursday 06 June 2013 09:21:35 Alex Deucher wrote:
> On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter wrote:
> > On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher wrote:
> >> To me at least, it doesn't make sense that an encoder can clone
> >> itself.  If an encoder is already in use, trying to clone itself would
> >> only lead to confusion and possible bugs (make sure some code path
> >> doesn't try and reprogram the encoder again, etc.).
> > 
> > For me the possible_clones mask is just the set of encoders which can
> > together share a crtc (presuming that crtc is indeed in all of the
> > possible_crtcs mask of each encoder). From that pov it makes imo sense
> > that a given encoder itself can always be with itself on the same crtc
> > ;-)
> > 
> > Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
> > internally use them, but it's not interface). And the possible_clones
> > stuff is by far not enough to describe all hw restrictions. So tbh I
> > don't care which way we go (or whether we indeed keep on using this
> > much at all).
> 
> Same.  I can go either way.

So what's the agreement ? :-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Thursday 06 June 2013 09:21:35 Alex Deucher wrote:
 On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter wrote:
  On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher wrote:
  To me at least, it doesn't make sense that an encoder can clone
  itself.  If an encoder is already in use, trying to clone itself would
  only lead to confusion and possible bugs (make sure some code path
  doesn't try and reprogram the encoder again, etc.).
  
  For me the possible_clones mask is just the set of encoders which can
  together share a crtc (presuming that crtc is indeed in all of the
  possible_crtcs mask of each encoder). From that pov it makes imo sense
  that a given encoder itself can always be with itself on the same crtc
  ;-)
  
  Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
  internally use them, but it's not interface). And the possible_clones
  stuff is by far not enough to describe all hw restrictions. So tbh I
  don't care which way we go (or whether we indeed keep on using this
  much at all).
 
 Same.  I can go either way.

So what's the agreement ? :-)

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

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

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

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

[Daniel Vetter]

On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote:
 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.

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

 - 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 

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

[Daniel Vetter]

On Fri, Jun 07, 2013 at 09:23:58AM +0200, Laurent Pinchart wrote:
 On Thursday 06 June 2013 09:21:35 Alex Deucher wrote:
  On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter wrote:
   On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher wrote:
   To me at least, it doesn't make sense that an encoder can clone
   itself.  If an encoder is already in use, trying to clone itself would
   only lead to confusion and possible bugs (make sure some code path
   doesn't try and reprogram the encoder again, etc.).
   
   For me the possible_clones mask is just the set of encoders which can
   together share a crtc (presuming that crtc is indeed in all of the
   possible_crtcs mask of each encoder). From that pov it makes imo sense
   that a given encoder itself can always be with itself on the same crtc
   ;-)
   
   Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
   internally use them, but it's not interface). And the possible_clones
   stuff is by far not enough to describe all hw restrictions. So tbh I
   don't care which way we go (or whether we indeed keep on using this
   much at all).
  
  Same.  I can go either way.
 
 So what's the agreement ? :-)

I think officially meh. Keep your current code and we'll try to fix this
once it blows up somewhere for real ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher  wrote:
> To me at least, it doesn't make sense that an encoder can clone
> itself.  If an encoder is already in use, trying to clone itself would
> only lead to confusion and possible bugs (make sure some code path
> doesn't try and reprogram the encoder again, etc.).

For me the possible_clones mask is just the set of encoders which can
together share a crtc (presuming that crtc is indeed in all of the
possible_crtcs mask of each encoder). From that pov it makes imo sense
that a given encoder itself can always be with itself on the same crtc
;-)

Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
internally use them, but it's not interface). And the possible_clones
stuff is by far not enough to describe all hw restrictions. So tbh I
don't care which way we go (or whether we indeed keep on using this
much at all).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Alex Deucher]

On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter  wrote:
> On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher  wrote:
>> To me at least, it doesn't make sense that an encoder can clone
>> itself.  If an encoder is already in use, trying to clone itself would
>> only lead to confusion and possible bugs (make sure some code path
>> doesn't try and reprogram the encoder again, etc.).
>
> For me the possible_clones mask is just the set of encoders which can
> together share a crtc (presuming that crtc is indeed in all of the
> possible_crtcs mask of each encoder). From that pov it makes imo sense
> that a given encoder itself can always be with itself on the same crtc
> ;-)
>
> Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
> internally use them, but it's not interface). And the possible_clones
> stuff is by far not enough to describe all hw restrictions. So tbh I
> don't care which way we go (or whether we indeed keep on using this
> much at all).

Same.  I can go either way.

Alex

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher alexdeuc...@gmail.com wrote:
 To me at least, it doesn't make sense that an encoder can clone
 itself.  If an encoder is already in use, trying to clone itself would
 only lead to confusion and possible bugs (make sure some code path
 doesn't try and reprogram the encoder again, etc.).

For me the possible_clones mask is just the set of encoders which can
together share a crtc (presuming that crtc is indeed in all of the
possible_crtcs mask of each encoder). From that pov it makes imo sense
that a given encoder itself can always be with itself on the same crtc
;-)

Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
internally use them, but it's not interface). And the possible_clones
stuff is by far not enough to describe all hw restrictions. So tbh I
don't care which way we go (or whether we indeed keep on using this
much at all).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Thu, Jun 6, 2013 at 9:12 AM, Daniel Vetter dan...@ffwll.ch wrote:
 On Wed, Jun 5, 2013 at 3:10 PM, Alex Deucher alexdeuc...@gmail.com wrote:
 To me at least, it doesn't make sense that an encoder can clone
 itself.  If an encoder is already in use, trying to clone itself would
 only lead to confusion and possible bugs (make sure some code path
 doesn't try and reprogram the encoder again, etc.).

 For me the possible_clones mask is just the set of encoders which can
 together share a crtc (presuming that crtc is indeed in all of the
 possible_crtcs mask of each encoder). From that pov it makes imo sense
 that a given encoder itself can always be with itself on the same crtc
 ;-)

 Otoh setcrtc doesn't care one bit about encoders (the crtc helpers do
 internally use them, but it's not interface). And the possible_clones
 stuff is by far not enough to describe all hw restrictions. So tbh I
 don't care which way we go (or whether we indeed keep on using this
 much at all).

Same.  I can go either way.

Alex

 -Daniel
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> 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]
> > 
> > >> Should we add that to crtc helpers, instead of the current "just try to
> > >> smash the old config on top of the ill-defined hw state after a failed
> > >> modeset"?
> > > 
> > > It would probably make sense to add a rollback operation to undo the
> > > prepare operation, or maybe just a rollback/commit flag to the commit
> > > operation. We would still need to smash the old config back though, as
> > > the rollback operation shouldn't be expected to handle encoders and
> > > connectors.
> > > 
> > > While we're at it, shouldn't we make drivers report supported formats for
> > > the main frame buffer, like we do for planes ? That would allow catching
> > > format errors before calling the prepare operation.
> > 
> > Yeah, I've noticed that one, too. I guess we could tackle that as part
> > of an eventual "make the implicit primary plane a bit more explict"
> > project. For now I'm not too offended by the duplication of checks.
> 
> It would be nice to treat the primary plane as all the other planes. Several 
> embedded display engines don't make the primary plane special and just paint 
> the background with a plain color when the enabled planes don't cover the 
> entire display.

Same deal for some intel hardware (at least partially). Anyways, my
current plan is that we fix it for the atomic pageflip/modeset stuff.
Ie. I don't even want expose the CRTC scanout stuff in the new atomic
API.

-- 
Ville Syrj?l?
Intel OTC

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

[Daniel Vetter]

On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> 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. 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.
- Adding fallback modes for connectors in the unknonw state (imo too much
  risk in breaking something else).

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

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Alex Deucher]

On Tue, Jun 4, 2013 at 9:51 PM, Laurent Pinchart
 wrote:
> Hi Daniel,
>
> 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]
>>
>> >> Should we add that to crtc helpers, instead of the current "just try to
>> >> smash the old config on top of the ill-defined hw state after a failed
>> >> modeset"?
>> >
>> > It would probably make sense to add a rollback operation to undo the
>> > prepare operation, or maybe just a rollback/commit flag to the commit
>> > operation. We would still need to smash the old config back though, as
>> > the rollback operation shouldn't be expected to handle encoders and
>> > connectors.
>> >
>> > While we're at it, shouldn't we make drivers report supported formats for
>> > the main frame buffer, like we do for planes ? That would allow catching
>> > format errors before calling the prepare operation.
>>
>> Yeah, I've noticed that one, too. I guess we could tackle that as part
>> of an eventual "make the implicit primary plane a bit more explict"
>> project. For now I'm not too offended by the duplication of checks.
>
> It would be nice to treat the primary plane as all the other planes. Several
> embedded display engines don't make the primary plane special and just paint
> the background with a plain color when the enabled planes don't cover the
> entire display.
>
>> >> This should use the drm_send_vblank_event helper.
>> >
>> > What bothers me about drm_send_vblank_event() is that it calls
>> > drm_vblank_count_and_time() with the events lock unnecessarily held. I can
>> > live with that for now, I'll fix the driver to use the helper.
>>
>> Most other drivers protect a bit of other state with that lock, so
>> makes sense to hold it outside already. So not sure whether we should
>> optimize this much ...
>
> Probably not :-)
>
>> >> > +   drm_vblank_put(dev, rcrtc->index);
>> >> > +}
>> >
>> > [snip]
>> >
>> >> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
>> >> > index 000..003b34e
>> >> > --- /dev/null
>> >> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >
>> > [snip]
>> >
>> >> > +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
>> >> > +{
>> >> > +   struct rcar_du_device *rcdu = dev->dev_private;
>> >> > +
>> >> > +   rcar_du_crtc_enable_vblank(>crtcs[crtc], false);
>> >> > +}
>> >>
>> >> Blergh, I hate our legacy vblank code which forces kms driver to jump
>> >> through int pipe -> struct drm_crtc * hoops.
>> >
>> > How would you like to fix it ? :-)
>>
>> Haven't looked at the details, but the first step I have in mind is to
>> switch all drm core -> driver and driver -> vblank helper interfaces from
>> int pipe to struct drm_crtc * pointers for kms drivers. That would allow us
>> to implement at least sane locking for the vblank wait ioctl (by grabbing
>> the crtc mutex).
>>
>> My plan was to split things by copy new kms functions and then
>> garbage-collecting all unnused features for the ums code (iirc no ums driver
>> ever supported more than 2 crtcs, vblank events or high-precision
>> timestamps).
>>
>> Once that's in place we can look into more stuff. One of the things I want
>> to play with is support for hw timestamp and vblank counters (also for
>> pageflips). Then we don't have to enable the vblank interrupt so often and
>> more important should be able to turn it of right away without loosing
>> precision due to the potential vblank irq vs. vblank irq off race.
>>
>> >> where i counts encoders to say that you can clone itself (userspace might
>> >> get confused, haven't checked how throughout the modeset ddx is). But it
>> >> sounds like rcar can clone encoders pretty freely (as long as they're
>> >> using crtc 0), so maybe you want to use something like drm/i915 does?
>> >
>> > The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0
>> > only, and output 1 can be driven by CRTC 0 or CRTC 1.
>>
>> Ah, that explains it, I've missed the context that we only have 2
>> crtc/encoder pairs ;-)
>
> It wasn't particularly explicit :-)
>
>> >> We smash all cloneable encoders into one groub with a
>> >> intel_encoder->cloneable flag and then allow cloning any cloneable
>> >> encoder to any other cloneable encoder with intel_encoder_clones in
>> >> intel_display.c
>> >>
>> >> possible_clones is a bit a ill-defined part of the kms api, but I think
>> >> we still should strive for consistency. Maybe the modesetting ddx should
>> >> also grow a warning if the possible_clones mask doesn't make too much
>> >> sense.
>> >
>> > I haven't been able to find an authoritative source of documentation
>> > regarding whether the possible_clones field should include the encoder
>> > itself. That should definitely be documented, I can fix the driver
>> > accordingly.
>>

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

[Laurent Pinchart]

Hi Daniel,

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]
> 
> >> Should we add that to crtc helpers, instead of the current "just try to
> >> smash the old config on top of the ill-defined hw state after a failed
> >> modeset"?
> > 
> > It would probably make sense to add a rollback operation to undo the
> > prepare operation, or maybe just a rollback/commit flag to the commit
> > operation. We would still need to smash the old config back though, as
> > the rollback operation shouldn't be expected to handle encoders and
> > connectors.
> > 
> > While we're at it, shouldn't we make drivers report supported formats for
> > the main frame buffer, like we do for planes ? That would allow catching
> > format errors before calling the prepare operation.
> 
> Yeah, I've noticed that one, too. I guess we could tackle that as part
> of an eventual "make the implicit primary plane a bit more explict"
> project. For now I'm not too offended by the duplication of checks.

It would be nice to treat the primary plane as all the other planes. Several 
embedded display engines don't make the primary plane special and just paint 
the background with a plain color when the enabled planes don't cover the 
entire display.

> >> This should use the drm_send_vblank_event helper.
> > 
> > What bothers me about drm_send_vblank_event() is that it calls
> > drm_vblank_count_and_time() with the events lock unnecessarily held. I can
> > live with that for now, I'll fix the driver to use the helper.
> 
> Most other drivers protect a bit of other state with that lock, so
> makes sense to hold it outside already. So not sure whether we should
> optimize this much ...

Probably not :-)

> >> > +   drm_vblank_put(dev, rcrtc->index);
> >> > +}
> > 
> > [snip]
> > 
> >> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
> >> > index 000..003b34e
> >> > --- /dev/null
> >> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > 
> > [snip]
> > 
> >> > +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
> >> > +{
> >> > +   struct rcar_du_device *rcdu = dev->dev_private;
> >> > +
> >> > +   rcar_du_crtc_enable_vblank(>crtcs[crtc], false);
> >> > +}
> >> 
> >> Blergh, I hate our legacy vblank code which forces kms driver to jump
> >> through int pipe -> struct drm_crtc * hoops.
> > 
> > How would you like to fix it ? :-)
> 
> Haven't looked at the details, but the first step I have in mind is to
> switch all drm core -> driver and driver -> vblank helper interfaces from
> int pipe to struct drm_crtc * pointers for kms drivers. That would allow us
> to implement at least sane locking for the vblank wait ioctl (by grabbing
> the crtc mutex).
> 
> My plan was to split things by copy new kms functions and then
> garbage-collecting all unnused features for the ums code (iirc no ums driver
> ever supported more than 2 crtcs, vblank events or high-precision
> timestamps).
> 
> Once that's in place we can look into more stuff. One of the things I want
> to play with is support for hw timestamp and vblank counters (also for
> pageflips). Then we don't have to enable the vblank interrupt so often and
> more important should be able to turn it of right away without loosing
> precision due to the potential vblank irq vs. vblank irq off race.
> 
> >> where i counts encoders to say that you can clone itself (userspace might
> >> get confused, haven't checked how throughout the modeset ddx is). But it
> >> sounds like rcar can clone encoders pretty freely (as long as they're
> >> using crtc 0), so maybe you want to use something like drm/i915 does?
> > 
> > The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0
> > only, and output 1 can be driven by CRTC 0 or CRTC 1.
> 
> Ah, that explains it, I've missed the context that we only have 2
> crtc/encoder pairs ;-)

It wasn't particularly explicit :-)

> >> We smash all cloneable encoders into one groub with a
> >> intel_encoder->cloneable flag and then allow cloning any cloneable
> >> encoder to any other cloneable encoder with intel_encoder_clones in
> >> intel_display.c
> >> 
> >> possible_clones is a bit a ill-defined part of the kms api, but I think
> >> we still should strive for consistency. Maybe the modesetting ddx should
> >> also grow a warning if the possible_clones mask doesn't make too much
> >> sense.
> > 
> > I haven't been able to find an authoritative source of documentation
> > regarding whether the possible_clones field should include the encoder
> > itself. That should definitely be documented, I can fix the driver
> > accordingly.
>
> Yeah, sounds like something worth clarifying. I'd vote for the self-clone
> bit to be set (I'm biased though, that's what i915 does). I guess we could
> 

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

[Daniel Vetter]

On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote:
 Hi Daniel,
 
 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. 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.
- Adding fallback modes for connectors in the unknonw state (imo too much
  risk in breaking something else).

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

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote:
 Hi Daniel,
 
 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]
  
   Should we add that to crtc helpers, instead of the current just try to
   smash the old config on top of the ill-defined hw state after a failed
   modeset?
   
   It would probably make sense to add a rollback operation to undo the
   prepare operation, or maybe just a rollback/commit flag to the commit
   operation. We would still need to smash the old config back though, as
   the rollback operation shouldn't be expected to handle encoders and
   connectors.
   
   While we're at it, shouldn't we make drivers report supported formats for
   the main frame buffer, like we do for planes ? That would allow catching
   format errors before calling the prepare operation.
  
  Yeah, I've noticed that one, too. I guess we could tackle that as part
  of an eventual make the implicit primary plane a bit more explict
  project. For now I'm not too offended by the duplication of checks.
 
 It would be nice to treat the primary plane as all the other planes. Several 
 embedded display engines don't make the primary plane special and just paint 
 the background with a plain color when the enabled planes don't cover the 
 entire display.

Same deal for some intel hardware (at least partially). Anyways, my
current plan is that we fix it for the atomic pageflip/modeset stuff.
Ie. I don't even want expose the CRTC scanout stuff in the new atomic
API.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Tue, Jun 4, 2013 at 9:51 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Daniel,

 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]

  Should we add that to crtc helpers, instead of the current just try to
  smash the old config on top of the ill-defined hw state after a failed
  modeset?
 
  It would probably make sense to add a rollback operation to undo the
  prepare operation, or maybe just a rollback/commit flag to the commit
  operation. We would still need to smash the old config back though, as
  the rollback operation shouldn't be expected to handle encoders and
  connectors.
 
  While we're at it, shouldn't we make drivers report supported formats for
  the main frame buffer, like we do for planes ? That would allow catching
  format errors before calling the prepare operation.

 Yeah, I've noticed that one, too. I guess we could tackle that as part
 of an eventual make the implicit primary plane a bit more explict
 project. For now I'm not too offended by the duplication of checks.

 It would be nice to treat the primary plane as all the other planes. Several
 embedded display engines don't make the primary plane special and just paint
 the background with a plain color when the enabled planes don't cover the
 entire display.

  This should use the drm_send_vblank_event helper.
 
  What bothers me about drm_send_vblank_event() is that it calls
  drm_vblank_count_and_time() with the events lock unnecessarily held. I can
  live with that for now, I'll fix the driver to use the helper.

 Most other drivers protect a bit of other state with that lock, so
 makes sense to hold it outside already. So not sure whether we should
 optimize this much ...

 Probably not :-)

   +   drm_vblank_put(dev, rcrtc-index);
   +}
 
  [snip]
 
   diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
   b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
   index 000..003b34e
   --- /dev/null
   +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
 
  [snip]
 
   +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
   +{
   +   struct rcar_du_device *rcdu = dev-dev_private;
   +
   +   rcar_du_crtc_enable_vblank(rcdu-crtcs[crtc], false);
   +}
 
  Blergh, I hate our legacy vblank code which forces kms driver to jump
  through int pipe - struct drm_crtc * hoops.
 
  How would you like to fix it ? :-)

 Haven't looked at the details, but the first step I have in mind is to
 switch all drm core - driver and driver - vblank helper interfaces from
 int pipe to struct drm_crtc * pointers for kms drivers. That would allow us
 to implement at least sane locking for the vblank wait ioctl (by grabbing
 the crtc mutex).

 My plan was to split things by copypasting new kms functions and then
 garbage-collecting all unnused features for the ums code (iirc no ums driver
 ever supported more than 2 crtcs, vblank events or high-precision
 timestamps).

 Once that's in place we can look into more stuff. One of the things I want
 to play with is support for hw timestamp and vblank counters (also for
 pageflips). Then we don't have to enable the vblank interrupt so often and
 more important should be able to turn it of right away without loosing
 precision due to the potential vblank irq vs. vblank irq off race.

  where i counts encoders to say that you can clone itself (userspace might
  get confused, haven't checked how throughout the modeset ddx is). But it
  sounds like rcar can clone encoders pretty freely (as long as they're
  using crtc 0), so maybe you want to use something like drm/i915 does?
 
  The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0
  only, and output 1 can be driven by CRTC 0 or CRTC 1.

 Ah, that explains it, I've missed the context that we only have 2
 crtc/encoder pairs ;-)

 It wasn't particularly explicit :-)

  We smash all cloneable encoders into one groub with a
  intel_encoder-cloneable flag and then allow cloning any cloneable
  encoder to any other cloneable encoder with intel_encoder_clones in
  intel_display.c
 
  possible_clones is a bit a ill-defined part of the kms api, but I think
  we still should strive for consistency. Maybe the modesetting ddx should
  also grow a warning if the possible_clones mask doesn't make too much
  sense.
 
  I haven't been able to find an authoritative source of documentation
  regarding whether the possible_clones field should include the encoder
  itself. That should definitely be documented, I can fix the driver
  accordingly.

 Yeah, sounds like something worth clarifying. I'd vote for the self-clone
 bit to be set (I'm biased though, that's what i915 does). I guess we could
 even enforce consistency by putting this into the drm encoders.

 I don't have a strong opinion on the color of this particular bikeshed, 

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

[Daniel Vetter]

On Tue, Jun 4, 2013 at 8:03 PM, Laurent Pinchart
 wrote:
> Hi Daniel,
>
> 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]

>> Should we add that to crtc helpers, instead of the current "just try to
>> smash the old config on top of the ill-defined hw state after a failed
>> modeset"?
>
> It would probably make sense to add a rollback operation to undo the prepare
> operation, or maybe just a rollback/commit flag to the commit operation. We
> would still need to smash the old config back though, as the rollback
> operation shouldn't be expected to handle encoders and connectors.
>
> While we're at it, shouldn't we make drivers report supported formats for the
> main frame buffer, like we do for planes ? That would allow catching format
> errors before calling the prepare operation.

Yeah, I've noticed that one, too. I guess we could tackle that as part
of an eventual "make the implicit primary plane a bit more explict"
project. For now I'm not too offended by the duplication of checks.


>> This should use the drm_send_vblank_event helper.
>
> What bothers me about drm_send_vblank_event() is that it calls
> drm_vblank_count_and_time() with the events lock unnecessarily held. I can
> live with that for now, I'll fix the driver to use the helper.

Most other drivers protect a bit of other state with that lock, so
makes sense to hold it outside already. So not sure whether we should
optimize this much ...

>> > +   drm_vblank_put(dev, rcrtc->index);
>> > +}
>
> [snip]
>
>> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
>> > index 000..003b34e
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>
> [snip]
>
>> > +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
>> > +{
>> > +   struct rcar_du_device *rcdu = dev->dev_private;
>> > +
>> > +   rcar_du_crtc_enable_vblank(>crtcs[crtc], false);
>> > +}
>>
>> Blergh, I hate our legacy vblank code which forces kms driver to jump
>> through int pipe -> struct drm_crtc * hoops.
>
> How would you like to fix it ? :-)

Haven't looked at the details, but the first step I have in mind is to
switch all drm core -> driver and driver -> vblank helper interfaces
from int pipe to struct drm_crtc * pointers for kms drivers. That
would allow us to implement at least sane locking for the vblank wait
ioctl (by grabbing the crtc mutex).

My plan was to split things by copy new kms functions and then
garbage-collecting all unnused features for the ums code (iirc no ums
driver ever supported more than 2 crtcs, vblank events or
high-precision timestamps).

Once that's in place we can look into more stuff. One of the things I
want to play with is support for hw timestamp and vblank counters
(also for pageflips). Then we don't have to enable the vblank
interrupt so often and more important should be able to turn it of
right away without loosing precision due to the potential vblank irq
vs. vblank irq off race.

>> where i counts encoders to say that you can clone itself (userspace might
>> get confused, haven't checked how throughout the modeset ddx is). But it
>> sounds like rcar can clone encoders pretty freely (as long as they're
>> using crtc 0), so maybe you want to use something like drm/i915 does?
>
> The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0 only,
> and output 1 can be driven by CRTC 0 or CRTC 1.

Ah, that explains it, I've missed the context that we only have 2
crtc/encoder pairs ;-)

>> We smash all cloneable encoders into one groub with a
>> intel_encoder->cloneable flag and then allow cloning any cloneable encoder
>> to any other cloneable encoder with intel_encoder_clones in
>> intel_display.c
>>
>> possible_clones is a bit a ill-defined part of the kms api, but I think we
>> still should strive for consistency. Maybe the modesetting ddx should also
>> grow a warning if the possible_clones mask doesn't make too much sense.
>
> I haven't been able to find an authoritative source of documentation regarding
> whether the possible_clones field should include the encoder itself. That
> should definitely be documented, I can fix the driver accordingly.

Yeah, sounds like something worth clarifying. I'd vote for the
self-clone bit to be set (I'm biased though, that's what i915 does). I
guess we could even enforce consistency by putting this into the drm
encoders.

Since the modesetting driver seems to not care too much I guess we can
fix that later on, imo not something to block merging rcar on.

[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 

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

[Laurent Pinchart]

Hi Daniel,

On Tuesday 04 June 2013 16:12:36 Daniel Vetter wrote:
> On Tue, Jun 04, 2013 at 04:53:40AM +0200, Laurent Pinchart wrote:
> > The R-Car Display Unit (DU) DRM driver supports both superposition
> > processors and all eight planes in RGB and YUV formats with alpha
> > blending.
> > 
> > Only VGA and LVDS encoders and connectors are currently supported.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Ok, I've done a little review, and the driver looks rather nice.

Thank you.

> With a simpler driver like this the drm boilerplate sticks out more, so I've
> dropped a few grumblings about that. But I've also spotted 3 little things
> which imo should be fixed before merging. Comments inline below.
> 
> Cheers, Daniel

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c new file mode 100644
> > index 000..c66fa4c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c

[snip]

> > +static void rcar_du_start_stop(struct rcar_du_device *rcdu, bool start)
> > +{
> > +   /* Many of the configuration bits are only updated when the display
> > +* reset (DRES) bit in DSYSR is set to 1, disabling *both* CRTCs.
> > +* Some of those bits could be pre-configured, but others (especially
> > +* the bits related to plane assignment to display timing
> > +* controllers) need to be modified at runtime.
> > +*
> > +* Restart the display controller if a start is requested. Sorry for 
> > +* the flicker. It should be possible to move most of the "DRES-
> > +* update" bits setup to driver initialization time and minimize the
> > +* number of cases when the display controller will have to be
> > +* restarted.
> > +*/
> > +   if (start) {
> > +   if (rcdu->used_crtcs++ != 0)
> > +   __rcar_du_start_stop(rcdu, false);
> > +   __rcar_du_start_stop(rcdu, true);
> > +   } else {
> > +   if (--rcdu->used_crtcs == 0)
> > +   __rcar_du_start_stop(rcdu, false);
> > +   }
> > +}
> 
> You seem to be a prime user for atomic modeset stuff ;-) Have you looked
> already a bit into sensible additions for the crtc helpers to make that
> possible? Maybe a global modeset_prepare/commit hook?

Not yet. That's somewhere in my to-do list, but it's growing too long :-( I 
need to finish CDF first.

[snip]

> > +static int rcar_du_crtc_mode_set(struct drm_crtc *crtc,
> > +struct drm_display_mode *mode,
> > +struct drm_display_mode *adjusted_mode,
> > +int x, int y,
> > +struct drm_framebuffer *old_fb)
> > +{
> > +   struct rcar_du_device *rcdu = crtc->dev->dev_private;
> > +   struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > +   const struct rcar_du_format_info *format;
> > +   int ret;
> > +
> > +   format = rcar_du_format_info(crtc->fb->pixel_format);
> > +   if (format == NULL) {
> > +   dev_dbg(rcdu->dev, "mode_set: unsupported format %08x\n",
> > +   crtc->fb->pixel_format);
> > +   ret = -EINVAL;
> > +   goto error;
> > +   }
> > +
> > +   ret = rcar_du_plane_reserve(rcrtc->plane, format);
> > +   if (ret < 0)
> > +   goto error;
> > +
> > +   rcrtc->plane->format = format;
> > +   rcrtc->plane->pitch = crtc->fb->pitches[0];
> > +
> > +   rcrtc->plane->src_x = x;
> > +   rcrtc->plane->src_y = y;
> > +   rcrtc->plane->width = mode->hdisplay;
> > +   rcrtc->plane->height = mode->vdisplay;
> > +
> > +   rcar_du_plane_compute_base(rcrtc->plane, crtc->fb);
> > +
> > +   rcrtc->outputs = 0;
> > +
> > +   return 0;
> > +
> > +error:
> > +   /* There's no rollback/abort operation to clean up in case of error. 
> > +* We thus need to release the reference to the DU acquired in
> > +* prepare() here.
> > +*/
> 
> Should we add that to crtc helpers, instead of the current "just try to
> smash the old config on top of the ill-defined hw state after a failed
> modeset"?

It would probably make sense to add a rollback operation to undo the prepare 
operation, or maybe just a rollback/commit flag to the commit operation. We 
would still need to smash the old config back though, as the rollback 
operation shouldn't be expected to handle encoders and connectors.

While we're at it, shouldn't we make drivers report supported formats for the 
main frame buffer, like we do for planes ? That would allow catching format 
errors before calling the prepare operation.

> > +   rcar_du_put(rcdu);
> > +   return ret;
> > +}

[snip]

> > +static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
> > +{
> > +   struct drm_pending_vblank_event *event;
> > +   struct drm_device *dev = rcrtc->crtc.dev;
> > +   struct timeval vblanktime;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>event_lock, flags);
> > +   event = rcrtc->event;
> > +   rcrtc->event 

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

[Daniel Vetter]

On Tue, Jun 04, 2013 at 04:53:40AM +0200, Laurent Pinchart wrote:
> The R-Car Display Unit (DU) DRM driver supports both superposition
> processors and all eight planes in RGB and YUV formats with alpha
> blending.
> 
> Only VGA and LVDS encoders and connectors are currently supported.
> 
> Signed-off-by: Laurent Pinchart 

Ok, I've done a little review, and the driver looks rather nice. With a
simpler driver like this the drm boilerplate sticks out more, so I've
dropped a few grumblings about that. But I've also spotted 3 little things
which imo should be fixed before merging. Comments inline below.

Cheers, Daniel

> ---
> Hi Dave,
> 
> There has been no comment on v2, so I'd like to get v3 in 3.11 is possible.
> The driver depends on the GEM CMA DMA-BUF patches I've sent earlier. If those
> can't make it to 3.11 I can sent a v4 with DRM PRIME support removed, and add
> it back for 3.12.
> 
> Changes since v1:
> 
>  - Use drm_encoder_cleanup() directly as .destroy handlers
>  - Enable alpha blending support
>  - Don't re-reserve hardware plane at each update
>  - Fix planes allocation for multiplanar formats
>  - Add DRM PRIME support
>  - Fix race condition between page flip request and handler
>  - Add configurable z-order support for planes
>  - Support configurable color keying for planes
>  - Update plane format after releasing hardware planes
>  - Fix register access for global registers
>  - Fix plane index wrap-around for multi-planar overlays
> 
> Changes since v2:
> 
>  - Enable the DE signal
>  - Split hardware and KMS planes
>  - Add support for the second CRTC
>  - Name the encoder platform data union
>  - Fix crash when disabling an already disabled plane
>  - Prepare/unprepare clock
>  - Centralize DU device core resource management
>  - Reorganize CRTC start/stop and power management code
>  - Create common encoder and connector structures
>  - Add support for cloned mode on DU1
>  - Add XRGB1555 format support
>  - Add plane property to set global alpha value
>  - Don't modify mode active size in encoder fixup
>  - Use the mode active size in mode set
>  - Take offsets into account in the mode_set_base handler
>  - Fix plane source position configuration
>  - Don't clean up mode setting if it hasn't been initialized
>  - Enable extended range for display timings
> 
>  drivers/gpu/drm/Kconfig |   2 +
>  drivers/gpu/drm/Makefile|   1 +
>  drivers/gpu/drm/rcar-du/Kconfig |   9 +
>  drivers/gpu/drm/rcar-du/Makefile|   8 +
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 602 
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  50 +++
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c   | 325 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  66 
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 245 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.h   |  59 
>  drivers/gpu/drm/rcar-du/rcar_du_lvds.c  | 216 
>  drivers/gpu/drm/rcar-du/rcar_du_lvds.h  |  24 ++
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 507 +++
>  drivers/gpu/drm/rcar-du/rcar_du_plane.h |  67 
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 445 +++
>  drivers/gpu/drm/rcar-du/rcar_du_vga.c   | 149 
>  drivers/gpu/drm/rcar-du/rcar_du_vga.h   |  24 ++
>  include/linux/platform_data/rcar-du.h   |  54 +++
>  18 files changed, 2853 insertions(+)
>  create mode 100644 drivers/gpu/drm/rcar-du/Kconfig
>  create mode 100644 drivers/gpu/drm/rcar-du/Makefile
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_drv.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_drv.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvds.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvds.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_plane.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_plane.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_regs.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vga.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vga.h
>  create mode 100644 include/linux/platform_data/rcar-du.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index b16c50e..71ca63b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -213,6 +213,8 @@ source "drivers/gpu/drm/mgag200/Kconfig"
>  
>  source "drivers/gpu/drm/cirrus/Kconfig"
>  
> +source "drivers/gpu/drm/rcar-du/Kconfig"
> +
>  source "drivers/gpu/drm/shmobile/Kconfig"
>  
>  source "drivers/gpu/drm/omapdrm/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1ecbe5b..801bcaf 100644
> --- a/drivers/gpu/drm/Makefile
> +++ 

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

[Laurent Pinchart]

The R-Car Display Unit (DU) DRM driver supports both superposition
processors and all eight planes in RGB and YUV formats with alpha
blending.

Only VGA and LVDS encoders and connectors are currently supported.

Signed-off-by: Laurent Pinchart 
---
Hi Dave,

There has been no comment on v2, so I'd like to get v3 in 3.11 is possible.
The driver depends on the GEM CMA DMA-BUF patches I've sent earlier. If those
can't make it to 3.11 I can sent a v4 with DRM PRIME support removed, and add
it back for 3.12.

Changes since v1:

 - Use drm_encoder_cleanup() directly as .destroy handlers
 - Enable alpha blending support
 - Don't re-reserve hardware plane at each update
 - Fix planes allocation for multiplanar formats
 - Add DRM PRIME support
 - Fix race condition between page flip request and handler
 - Add configurable z-order support for planes
 - Support configurable color keying for planes
 - Update plane format after releasing hardware planes
 - Fix register access for global registers
 - Fix plane index wrap-around for multi-planar overlays

Changes since v2:

 - Enable the DE signal
 - Split hardware and KMS planes
 - Add support for the second CRTC
 - Name the encoder platform data union
 - Fix crash when disabling an already disabled plane
 - Prepare/unprepare clock
 - Centralize DU device core resource management
 - Reorganize CRTC start/stop and power management code
 - Create common encoder and connector structures
 - Add support for cloned mode on DU1
 - Add XRGB1555 format support
 - Add plane property to set global alpha value
 - Don't modify mode active size in encoder fixup
 - Use the mode active size in mode set
 - Take offsets into account in the mode_set_base handler
 - Fix plane source position configuration
 - Don't clean up mode setting if it hasn't been initialized
 - Enable extended range for display timings

 drivers/gpu/drm/Kconfig |   2 +
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/rcar-du/Kconfig |   9 +
 drivers/gpu/drm/rcar-du/Makefile|   8 +
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 602 
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  50 +++
 drivers/gpu/drm/rcar-du/rcar_du_drv.c   | 325 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  66 
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 245 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.h   |  59 
 drivers/gpu/drm/rcar-du/rcar_du_lvds.c  | 216 
 drivers/gpu/drm/rcar-du/rcar_du_lvds.h  |  24 ++
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 507 +++
 drivers/gpu/drm/rcar-du/rcar_du_plane.h |  67 
 drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 445 +++
 drivers/gpu/drm/rcar-du/rcar_du_vga.c   | 149 
 drivers/gpu/drm/rcar-du/rcar_du_vga.h   |  24 ++
 include/linux/platform_data/rcar-du.h   |  54 +++
 18 files changed, 2853 insertions(+)
 create mode 100644 drivers/gpu/drm/rcar-du/Kconfig
 create mode 100644 drivers/gpu/drm/rcar-du/Makefile
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_crtc.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_crtc.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_drv.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_drv.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvds.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvds.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_plane.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_plane.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_regs.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vga.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vga.h
 create mode 100644 include/linux/platform_data/rcar-du.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b16c50e..71ca63b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -213,6 +213,8 @@ source "drivers/gpu/drm/mgag200/Kconfig"

 source "drivers/gpu/drm/cirrus/Kconfig"

+source "drivers/gpu/drm/rcar-du/Kconfig"
+
 source "drivers/gpu/drm/shmobile/Kconfig"

 source "drivers/gpu/drm/omapdrm/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1ecbe5b..801bcaf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_DRM_EXYNOS) +=exynos/
 obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
+obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-$(CONFIG_DRM_OMAP) += omapdrm/
 obj-$(CONFIG_DRM_TILCDC)   += tilcdc/
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
new file mode 100644
index 000..2eb7d23
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -0,0 +1,9 @@
+config DRM_RCAR_DU
+   tristate "DRM Support for R-Car 

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

[Laurent Pinchart]

Hi Daniel,

On Tuesday 04 June 2013 16:12:36 Daniel Vetter wrote:
 On Tue, Jun 04, 2013 at 04:53:40AM +0200, Laurent Pinchart wrote:
  The R-Car Display Unit (DU) DRM driver supports both superposition
  processors and all eight planes in RGB and YUV formats with alpha
  blending.
  
  Only VGA and LVDS encoders and connectors are currently supported.
  
  Signed-off-by: Laurent Pinchart
  laurent.pinchart+rene...@ideasonboard.com
 
 Ok, I've done a little review, and the driver looks rather nice.

Thank you.

 With a simpler driver like this the drm boilerplate sticks out more, so I've
 dropped a few grumblings about that. But I've also spotted 3 little things
 which imo should be fixed before merging. Comments inline below.
 
 Cheers, Daniel

[snip]

  diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
  b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c new file mode 100644
  index 000..c66fa4c
  --- /dev/null
  +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c

[snip]

  +static void rcar_du_start_stop(struct rcar_du_device *rcdu, bool start)
  +{
  +   /* Many of the configuration bits are only updated when the display
  +* reset (DRES) bit in DSYSR is set to 1, disabling *both* CRTCs.
  +* Some of those bits could be pre-configured, but others (especially
  +* the bits related to plane assignment to display timing
  +* controllers) need to be modified at runtime.
  +*
  +* Restart the display controller if a start is requested. Sorry for 
  +* the flicker. It should be possible to move most of the DRES-
  +* update bits setup to driver initialization time and minimize the
  +* number of cases when the display controller will have to be
  +* restarted.
  +*/
  +   if (start) {
  +   if (rcdu-used_crtcs++ != 0)
  +   __rcar_du_start_stop(rcdu, false);
  +   __rcar_du_start_stop(rcdu, true);
  +   } else {
  +   if (--rcdu-used_crtcs == 0)
  +   __rcar_du_start_stop(rcdu, false);
  +   }
  +}
 
 You seem to be a prime user for atomic modeset stuff ;-) Have you looked
 already a bit into sensible additions for the crtc helpers to make that
 possible? Maybe a global modeset_prepare/commit hook?

Not yet. That's somewhere in my to-do list, but it's growing too long :-( I 
need to finish CDF first.

[snip]

  +static int rcar_du_crtc_mode_set(struct drm_crtc *crtc,
  +struct drm_display_mode *mode,
  +struct drm_display_mode *adjusted_mode,
  +int x, int y,
  +struct drm_framebuffer *old_fb)
  +{
  +   struct rcar_du_device *rcdu = crtc-dev-dev_private;
  +   struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
  +   const struct rcar_du_format_info *format;
  +   int ret;
  +
  +   format = rcar_du_format_info(crtc-fb-pixel_format);
  +   if (format == NULL) {
  +   dev_dbg(rcdu-dev, mode_set: unsupported format %08x\n,
  +   crtc-fb-pixel_format);
  +   ret = -EINVAL;
  +   goto error;
  +   }
  +
  +   ret = rcar_du_plane_reserve(rcrtc-plane, format);
  +   if (ret  0)
  +   goto error;
  +
  +   rcrtc-plane-format = format;
  +   rcrtc-plane-pitch = crtc-fb-pitches[0];
  +
  +   rcrtc-plane-src_x = x;
  +   rcrtc-plane-src_y = y;
  +   rcrtc-plane-width = mode-hdisplay;
  +   rcrtc-plane-height = mode-vdisplay;
  +
  +   rcar_du_plane_compute_base(rcrtc-plane, crtc-fb);
  +
  +   rcrtc-outputs = 0;
  +
  +   return 0;
  +
  +error:
  +   /* There's no rollback/abort operation to clean up in case of error. 
  +* We thus need to release the reference to the DU acquired in
  +* prepare() here.
  +*/
 
 Should we add that to crtc helpers, instead of the current just try to
 smash the old config on top of the ill-defined hw state after a failed
 modeset?

It would probably make sense to add a rollback operation to undo the prepare 
operation, or maybe just a rollback/commit flag to the commit operation. We 
would still need to smash the old config back though, as the rollback 
operation shouldn't be expected to handle encoders and connectors.

While we're at it, shouldn't we make drivers report supported formats for the 
main frame buffer, like we do for planes ? That would allow catching format 
errors before calling the prepare operation.

  +   rcar_du_put(rcdu);
  +   return ret;
  +}

[snip]

  +static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
  +{
  +   struct drm_pending_vblank_event *event;
  +   struct drm_device *dev = rcrtc-crtc.dev;
  +   struct timeval vblanktime;
  +   unsigned long flags;
  +
  +   spin_lock_irqsave(dev-event_lock, flags);
  +   event = rcrtc-event;
  +   rcrtc-event = NULL;
  +   spin_unlock_irqrestore(dev-event_lock, flags);
  +
  +   if (event == NULL)
  +   return;
  +
  +   event-event.sequence =
  +   drm_vblank_count_and_time(dev, rcrtc-index, vblanktime);
  +   

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

[Daniel Vetter]

On Tue, Jun 4, 2013 at 8:03 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Daniel,

 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]

 Should we add that to crtc helpers, instead of the current just try to
 smash the old config on top of the ill-defined hw state after a failed
 modeset?

 It would probably make sense to add a rollback operation to undo the prepare
 operation, or maybe just a rollback/commit flag to the commit operation. We
 would still need to smash the old config back though, as the rollback
 operation shouldn't be expected to handle encoders and connectors.

 While we're at it, shouldn't we make drivers report supported formats for the
 main frame buffer, like we do for planes ? That would allow catching format
 errors before calling the prepare operation.

Yeah, I've noticed that one, too. I guess we could tackle that as part
of an eventual make the implicit primary plane a bit more explict
project. For now I'm not too offended by the duplication of checks.


 This should use the drm_send_vblank_event helper.

 What bothers me about drm_send_vblank_event() is that it calls
 drm_vblank_count_and_time() with the events lock unnecessarily held. I can
 live with that for now, I'll fix the driver to use the helper.

Most other drivers protect a bit of other state with that lock, so
makes sense to hold it outside already. So not sure whether we should
optimize this much ...

  +   drm_vblank_put(dev, rcrtc-index);
  +}

 [snip]

  diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
  b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
  index 000..003b34e
  --- /dev/null
  +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c

 [snip]

  +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
  +{
  +   struct rcar_du_device *rcdu = dev-dev_private;
  +
  +   rcar_du_crtc_enable_vblank(rcdu-crtcs[crtc], false);
  +}

 Blergh, I hate our legacy vblank code which forces kms driver to jump
 through int pipe - struct drm_crtc * hoops.

 How would you like to fix it ? :-)

Haven't looked at the details, but the first step I have in mind is to
switch all drm core - driver and driver - vblank helper interfaces
from int pipe to struct drm_crtc * pointers for kms drivers. That
would allow us to implement at least sane locking for the vblank wait
ioctl (by grabbing the crtc mutex).

My plan was to split things by copypasting new kms functions and then
garbage-collecting all unnused features for the ums code (iirc no ums
driver ever supported more than 2 crtcs, vblank events or
high-precision timestamps).

Once that's in place we can look into more stuff. One of the things I
want to play with is support for hw timestamp and vblank counters
(also for pageflips). Then we don't have to enable the vblank
interrupt so often and more important should be able to turn it of
right away without loosing precision due to the potential vblank irq
vs. vblank irq off race.

 where i counts encoders to say that you can clone itself (userspace might
 get confused, haven't checked how throughout the modeset ddx is). But it
 sounds like rcar can clone encoders pretty freely (as long as they're
 using crtc 0), so maybe you want to use something like drm/i915 does?

 The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0 only,
 and output 1 can be driven by CRTC 0 or CRTC 1.

Ah, that explains it, I've missed the context that we only have 2
crtc/encoder pairs ;-)

 We smash all cloneable encoders into one groub with a
 intel_encoder-cloneable flag and then allow cloning any cloneable encoder
 to any other cloneable encoder with intel_encoder_clones in
 intel_display.c

 possible_clones is a bit a ill-defined part of the kms api, but I think we
 still should strive for consistency. Maybe the modesetting ddx should also
 grow a warning if the possible_clones mask doesn't make too much sense.

 I haven't been able to find an authoritative source of documentation regarding
 whether the possible_clones field should include the encoder itself. That
 should definitely be documented, I can fix the driver accordingly.

Yeah, sounds like something worth clarifying. I'd vote for the
self-clone bit to be set (I'm biased though, that's what i915 does). I
guess we could even enforce consistency by putting this into the drm
encoders.

Since the modesetting driver seems to not care too much I guess we can
fix that later on, imo not something to block merging rcar on.

[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
 

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

[Laurent Pinchart]

Hi Daniel,

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]
 
  Should we add that to crtc helpers, instead of the current just try to
  smash the old config on top of the ill-defined hw state after a failed
  modeset?
  
  It would probably make sense to add a rollback operation to undo the
  prepare operation, or maybe just a rollback/commit flag to the commit
  operation. We would still need to smash the old config back though, as
  the rollback operation shouldn't be expected to handle encoders and
  connectors.
  
  While we're at it, shouldn't we make drivers report supported formats for
  the main frame buffer, like we do for planes ? That would allow catching
  format errors before calling the prepare operation.
 
 Yeah, I've noticed that one, too. I guess we could tackle that as part
 of an eventual make the implicit primary plane a bit more explict
 project. For now I'm not too offended by the duplication of checks.

It would be nice to treat the primary plane as all the other planes. Several 
embedded display engines don't make the primary plane special and just paint 
the background with a plain color when the enabled planes don't cover the 
entire display.

  This should use the drm_send_vblank_event helper.
  
  What bothers me about drm_send_vblank_event() is that it calls
  drm_vblank_count_and_time() with the events lock unnecessarily held. I can
  live with that for now, I'll fix the driver to use the helper.
 
 Most other drivers protect a bit of other state with that lock, so
 makes sense to hold it outside already. So not sure whether we should
 optimize this much ...

Probably not :-)

   +   drm_vblank_put(dev, rcrtc-index);
   +}
  
  [snip]
  
   diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
   b/drivers/gpu/drm/rcar-du/rcar_du_drv.c new file mode 100644
   index 000..003b34e
   --- /dev/null
   +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
  
  [snip]
  
   +static void rcar_du_disable_vblank(struct drm_device *dev, int crtc)
   +{
   +   struct rcar_du_device *rcdu = dev-dev_private;
   +
   +   rcar_du_crtc_enable_vblank(rcdu-crtcs[crtc], false);
   +}
  
  Blergh, I hate our legacy vblank code which forces kms driver to jump
  through int pipe - struct drm_crtc * hoops.
  
  How would you like to fix it ? :-)
 
 Haven't looked at the details, but the first step I have in mind is to
 switch all drm core - driver and driver - vblank helper interfaces from
 int pipe to struct drm_crtc * pointers for kms drivers. That would allow us
 to implement at least sane locking for the vblank wait ioctl (by grabbing
 the crtc mutex).
 
 My plan was to split things by copypasting new kms functions and then
 garbage-collecting all unnused features for the ums code (iirc no ums driver
 ever supported more than 2 crtcs, vblank events or high-precision
 timestamps).
 
 Once that's in place we can look into more stuff. One of the things I want
 to play with is support for hw timestamp and vblank counters (also for
 pageflips). Then we don't have to enable the vblank interrupt so often and
 more important should be able to turn it of right away without loosing
 precision due to the potential vblank irq vs. vblank irq off race.
 
  where i counts encoders to say that you can clone itself (userspace might
  get confused, haven't checked how throughout the modeset ddx is). But it
  sounds like rcar can clone encoders pretty freely (as long as they're
  using crtc 0), so maybe you want to use something like drm/i915 does?
  
  The device has two outputs, 0 and 1. Output 0 can be driven by CRTC 0
  only, and output 1 can be driven by CRTC 0 or CRTC 1.
 
 Ah, that explains it, I've missed the context that we only have 2
 crtc/encoder pairs ;-)

It wasn't particularly explicit :-)

  We smash all cloneable encoders into one groub with a
  intel_encoder-cloneable flag and then allow cloning any cloneable
  encoder to any other cloneable encoder with intel_encoder_clones in
  intel_display.c
  
  possible_clones is a bit a ill-defined part of the kms api, but I think
  we still should strive for consistency. Maybe the modesetting ddx should
  also grow a warning if the possible_clones mask doesn't make too much
  sense.
  
  I haven't been able to find an authoritative source of documentation
  regarding whether the possible_clones field should include the encoder
  itself. That should definitely be documented, I can fix the driver
  accordingly.

 Yeah, sounds like something worth clarifying. I'd vote for the self-clone
 bit to be set (I'm biased though, that's what i915 does). I guess we could
 even enforce consistency by putting this into the drm encoders.

I don't have a strong opinion on the color of this particular bikeshed, but it 
would be nice to hear what other developers think.

 Since the