[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Daniel Vetter]

On Tue, Nov 04, 2014 at 04:26:12PM +0100, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 10:11:22AM -0500, Sean Paul wrote:
> > On Tue, Nov 4, 2014 at 9:50 AM, Thierry Reding  > gmail.com> wrote:
> > > On Mon, Nov 03, 2014 at 12:18:30PM -0500, Sean Paul wrote:
> > >> On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding  > >> gmail.com> wrote:
> > >> > From: Thierry Reding 
> > >> >
> > >> > The output is already enabled in .dpms(), doing it in .mode_set() too
> > >> > can cause noticeable flicker.
> > >> >
> > >>
> > >> I think this should be coupled with "drm/tegra: DPMS off/on in encoder
> > >> prepare/commit" that I sent earlier this week. Without it, the driver
> > >> can get into a state where connector status is on, but the output is
> > >> disabled.
> > >
> > > I'm not sure I exactly understand which problem that patch fixes, but
> > > I'll give it some testing to see if it doesn't break anything.
> > >
> > 
> > I'll try to explain it more clearly.
> > 
> > The problem occurs when userspace does set_property(dpms_on), then modeset.
> > 
> > When the set_property ioctl to set dpms = on is called, the tegra
> > driver goes through drm_helper_connector_dpms(). At this point,
> > because the connector has not participated in a modeset,
> > connector->encoder == NULL. In drm_helper_connector_dpms(), we set
> > connector->dpms = DRM_MODE_DPMS_ON, and skip everything else because
> > !encoder && !crtc.
> > 
> > When modeset is called, we go through drm_crtc_helper_set_config().
> > This function will populate connector->encoder with the appropriate
> > encoder and call drm_crtc_helper_set_mode(), which goes through the
> > prepare/mode_set/commit calls. Finally, drm_crtc_helper_set_config()
> > iterates through the connectors involved in the modeset and calls
> > their dpms() func. In our case, as above, we go through
> > drm_helper_connector_dpms(), but it just exits early because mode ==
> > connector->dpms.
> > 
> > So we end up in a state where connector->dpms == DRM_MODE_DPMS_ON, and
> > the output, as well as any connected panel, has never been enabled.
> > The reason is that they're only enabled in encoder_funcs->dpms(),
> > which is never called in the above scenario.
> > 
> > I hope that makes more sense :-)
> 
> Yes, perfect sense. It's somewhat unfortunate that the helpers even
> allow this to happen. It doesn't seem like it's only the Tegra driver
> using it wrongly. Anyway, I've applied this to my tree.

The helper support for dpms is completely tacked onto the side and yeah
you get to do all the hard work of keeping track of state in the driver.

For modesets atomic should improve on the situation, except that I didn't
write any helpers to implement DPMS. Oops, totally missed that entry
point.

So maybe we should write new helpers to implement connector->dpms on top
of atomic which are actually sane?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Thierry Reding]

On Tue, Nov 04, 2014 at 10:11:22AM -0500, Sean Paul wrote:
> On Tue, Nov 4, 2014 at 9:50 AM, Thierry Reding  
> wrote:
> > On Mon, Nov 03, 2014 at 12:18:30PM -0500, Sean Paul wrote:
> >> On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding  >> gmail.com> wrote:
> >> > From: Thierry Reding 
> >> >
> >> > The output is already enabled in .dpms(), doing it in .mode_set() too
> >> > can cause noticeable flicker.
> >> >
> >>
> >> I think this should be coupled with "drm/tegra: DPMS off/on in encoder
> >> prepare/commit" that I sent earlier this week. Without it, the driver
> >> can get into a state where connector status is on, but the output is
> >> disabled.
> >
> > I'm not sure I exactly understand which problem that patch fixes, but
> > I'll give it some testing to see if it doesn't break anything.
> >
> 
> I'll try to explain it more clearly.
> 
> The problem occurs when userspace does set_property(dpms_on), then modeset.
> 
> When the set_property ioctl to set dpms = on is called, the tegra
> driver goes through drm_helper_connector_dpms(). At this point,
> because the connector has not participated in a modeset,
> connector->encoder == NULL. In drm_helper_connector_dpms(), we set
> connector->dpms = DRM_MODE_DPMS_ON, and skip everything else because
> !encoder && !crtc.
> 
> When modeset is called, we go through drm_crtc_helper_set_config().
> This function will populate connector->encoder with the appropriate
> encoder and call drm_crtc_helper_set_mode(), which goes through the
> prepare/mode_set/commit calls. Finally, drm_crtc_helper_set_config()
> iterates through the connectors involved in the modeset and calls
> their dpms() func. In our case, as above, we go through
> drm_helper_connector_dpms(), but it just exits early because mode ==
> connector->dpms.
> 
> So we end up in a state where connector->dpms == DRM_MODE_DPMS_ON, and
> the output, as well as any connected panel, has never been enabled.
> The reason is that they're only enabled in encoder_funcs->dpms(),
> which is never called in the above scenario.
> 
> I hope that makes more sense :-)

Yes, perfect sense. It's somewhat unfortunate that the helpers even
allow this to happen. It doesn't seem like it's only the Tegra driver
using it wrongly. Anyway, I've applied this to my tree.

Thanks,
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Thierry Reding]

On Mon, Nov 03, 2014 at 12:18:30PM -0500, Sean Paul wrote:
> On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding  
> wrote:
> > From: Thierry Reding 
> >
> > The output is already enabled in .dpms(), doing it in .mode_set() too
> > can cause noticeable flicker.
> >
> 
> I think this should be coupled with "drm/tegra: DPMS off/on in encoder
> prepare/commit" that I sent earlier this week. Without it, the driver
> can get into a state where connector status is on, but the output is
> disabled.

I'm not sure I exactly understand which problem that patch fixes, but
I'll give it some testing to see if it doesn't break anything.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Sean Paul]

On Tue, Nov 4, 2014 at 9:50 AM, Thierry Reding  
wrote:
> On Mon, Nov 03, 2014 at 12:18:30PM -0500, Sean Paul wrote:
>> On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding  
>> wrote:
>> > From: Thierry Reding 
>> >
>> > The output is already enabled in .dpms(), doing it in .mode_set() too
>> > can cause noticeable flicker.
>> >
>>
>> I think this should be coupled with "drm/tegra: DPMS off/on in encoder
>> prepare/commit" that I sent earlier this week. Without it, the driver
>> can get into a state where connector status is on, but the output is
>> disabled.
>
> I'm not sure I exactly understand which problem that patch fixes, but
> I'll give it some testing to see if it doesn't break anything.
>

I'll try to explain it more clearly.

The problem occurs when userspace does set_property(dpms_on), then modeset.

When the set_property ioctl to set dpms = on is called, the tegra
driver goes through drm_helper_connector_dpms(). At this point,
because the connector has not participated in a modeset,
connector->encoder == NULL. In drm_helper_connector_dpms(), we set
connector->dpms = DRM_MODE_DPMS_ON, and skip everything else because
!encoder && !crtc.

When modeset is called, we go through drm_crtc_helper_set_config().
This function will populate connector->encoder with the appropriate
encoder and call drm_crtc_helper_set_mode(), which goes through the
prepare/mode_set/commit calls. Finally, drm_crtc_helper_set_config()
iterates through the connectors involved in the modeset and calls
their dpms() func. In our case, as above, we go through
drm_helper_connector_dpms(), but it just exits early because mode ==
connector->dpms.

So we end up in a state where connector->dpms == DRM_MODE_DPMS_ON, and
the output, as well as any connected panel, has never been enabled.
The reason is that they're only enabled in encoder_funcs->dpms(),
which is never called in the above scenario.

I hope that makes more sense :-)

Sean


> Thierry

[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Sean Paul]

On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding  
wrote:
> From: Thierry Reding 
>
> The output is already enabled in .dpms(), doing it in .mode_set() too
> can cause noticeable flicker.
>

I think this should be coupled with "drm/tegra: DPMS off/on in encoder
prepare/commit" that I sent earlier this week. Without it, the driver
can get into a state where connector status is on, but the output is
disabled.

Sean


> Signed-off-by: Thierry Reding 
> ---
>  drivers/gpu/drm/tegra/output.c | 6 --
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 0c67d7eebc94..6b393cfbb5e7 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -167,12 +167,6 @@ static void tegra_encoder_mode_set(struct drm_encoder 
> *encoder,
>struct drm_display_mode *mode,
>struct drm_display_mode *adjusted)
>  {
> -   struct tegra_output *output = encoder_to_output(encoder);
> -   int err;
> -
> -   err = tegra_output_enable(output);
> -   if (err < 0)
> -   dev_err(encoder->dev->dev, "tegra_output_enable(): %d\n", 
> err);
>  }
>
>  static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> --
> 2.1.2
>

[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Thierry Reding]

From: Thierry Reding 

The output is already enabled in .dpms(), doing it in .mode_set() too
can cause noticeable flicker.

Signed-off-by: Thierry Reding 
---
 drivers/gpu/drm/tegra/output.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 0c67d7eebc94..6b393cfbb5e7 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -167,12 +167,6 @@ static void tegra_encoder_mode_set(struct drm_encoder 
*encoder,
   struct drm_display_mode *mode,
   struct drm_display_mode *adjusted)
 {
-   struct tegra_output *output = encoder_to_output(encoder);
-   int err;
-
-   err = tegra_output_enable(output);
-   if (err < 0)
-   dev_err(encoder->dev->dev, "tegra_output_enable(): %d\n", err);
 }

 static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
-- 
2.1.2