diff mbox

[1/1] drm/tegra: Add guard to avoid double disable/enable of RGB outputs

Message ID 1392138747-18198-1-git-send-email-digetx@gmail.com
State Awaiting Upstream
Delegated to: Thierry Reding
Headers show

Commit Message

Dmitry Osipenko Feb. 11, 2014, 5:12 p.m. UTC
Add guard to check whether RGB output is already enabled in the way it's
done for HDMI output. Fixes possible hang on trying to disable output twice
(first time during driver probe and second on fb registering).

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/rgb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Erik Faye-Lund Feb. 11, 2014, 7:13 p.m. UTC | #1
On Tue, Feb 11, 2014 at 6:12 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Add guard to check whether RGB output is already enabled in the way it's
> done for HDMI output. Fixes possible hang on trying to disable output twice
> (first time during driver probe and second on fb registering).
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/rgb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index 338f7f6..0266fb4 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -15,6 +15,7 @@
>  struct tegra_rgb {
>         struct tegra_output output;
>         struct tegra_dc *dc;
> +       bool enabled;
>
>         struct clk *clk_parent;
>         struct clk *clk;
> @@ -89,6 +90,9 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
>         struct tegra_rgb *rgb = to_rgb(output);
>         unsigned long value;
>
> +       if (rgb->enabled)
> +               return 0;
> +
>         tegra_dc_write_regs(rgb->dc, rgb_enable, ARRAY_SIZE(rgb_enable));
>
>         value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
> @@ -122,6 +126,8 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
>         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
>         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
>
> +       rgb->enabled = true;
> +
>         return 0;
>  }
>
> @@ -130,6 +136,9 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
>         struct tegra_rgb *rgb = to_rgb(output);
>         unsigned long value;
>
> +       if (!rgb->enabled)
> +               return 0;
> +
>         value = tegra_dc_readl(rgb->dc, DC_CMD_DISPLAY_POWER_CONTROL);
>         value &= ~(PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
>                    PW4_ENABLE | PM0_ENABLE | PM1_ENABLE);
> @@ -144,6 +153,8 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
>
>         tegra_dc_write_regs(rgb->dc, rgb_disable, ARRAY_SIZE(rgb_disable));
>
> +       rgb->enabled = false;
> +
>         return 0;
>  }
>

Wouldn't it make more sense to make "enabled" and int that counts how
many times tegra_output_rgb_enable has been called? That way you can
have tegra_output_rgb_disable only really disable the display once the
same amount of disables have been performed...
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Osipenko Feb. 11, 2014, 7:48 p.m. UTC | #2
11.02.2014 23:13, Erik Faye-Lund пишет:
> On Tue, Feb 11, 2014 at 6:12 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Add guard to check whether RGB output is already enabled in the way it's
>> done for HDMI output. Fixes possible hang on trying to disable output twice
>> (first time during driver probe and second on fb registering).
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/tegra/rgb.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
>> index 338f7f6..0266fb4 100644
>> --- a/drivers/gpu/drm/tegra/rgb.c
>> +++ b/drivers/gpu/drm/tegra/rgb.c
>> @@ -15,6 +15,7 @@
>>  struct tegra_rgb {
>>         struct tegra_output output;
>>         struct tegra_dc *dc;
>> +       bool enabled;
>>
>>         struct clk *clk_parent;
>>         struct clk *clk;
>> @@ -89,6 +90,9 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
>>         struct tegra_rgb *rgb = to_rgb(output);
>>         unsigned long value;
>>
>> +       if (rgb->enabled)
>> +               return 0;
>> +
>>         tegra_dc_write_regs(rgb->dc, rgb_enable, ARRAY_SIZE(rgb_enable));
>>
>>         value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
>> @@ -122,6 +126,8 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
>>         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
>>         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
>>
>> +       rgb->enabled = true;
>> +
>>         return 0;
>>  }
>>
>> @@ -130,6 +136,9 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
>>         struct tegra_rgb *rgb = to_rgb(output);
>>         unsigned long value;
>>
>> +       if (!rgb->enabled)
>> +               return 0;
>> +
>>         value = tegra_dc_readl(rgb->dc, DC_CMD_DISPLAY_POWER_CONTROL);
>>         value &= ~(PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
>>                    PW4_ENABLE | PM0_ENABLE | PM1_ENABLE);
>> @@ -144,6 +153,8 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
>>
>>         tegra_dc_write_regs(rgb->dc, rgb_disable, ARRAY_SIZE(rgb_disable));
>>
>> +       rgb->enabled = false;
>> +
>>         return 0;
>>  }
>>
> 
> Wouldn't it make more sense to make "enabled" and int that counts how
> many times tegra_output_rgb_enable has been called? That way you can
> have tegra_output_rgb_disable only really disable the display once the
> same amount of disables have been performed...
> 
RGB is a part of DC, i.e. one per DC. tegra_output_rgb_enable() would be called
for each individual DC, so it doesn't make any sense. Besides, I doubt tegra
device with two panels ever existed or will be :)
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Feb. 11, 2014, 8:54 p.m. UTC | #3
On Tue, Feb 11, 2014 at 08:13:14PM +0100, Erik Faye-Lund wrote:
> On Tue, Feb 11, 2014 at 6:12 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> > Add guard to check whether RGB output is already enabled in the way it's
> > done for HDMI output. Fixes possible hang on trying to disable output twice
> > (first time during driver probe and second on fb registering).
> >
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/gpu/drm/tegra/rgb.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> > index 338f7f6..0266fb4 100644
> > --- a/drivers/gpu/drm/tegra/rgb.c
> > +++ b/drivers/gpu/drm/tegra/rgb.c
> > @@ -15,6 +15,7 @@
> >  struct tegra_rgb {
> >         struct tegra_output output;
> >         struct tegra_dc *dc;
> > +       bool enabled;
> >
> >         struct clk *clk_parent;
> >         struct clk *clk;
> > @@ -89,6 +90,9 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
> >         struct tegra_rgb *rgb = to_rgb(output);
> >         unsigned long value;
> >
> > +       if (rgb->enabled)
> > +               return 0;
> > +
> >         tegra_dc_write_regs(rgb->dc, rgb_enable, ARRAY_SIZE(rgb_enable));
> >
> >         value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
> > @@ -122,6 +126,8 @@ static int tegra_output_rgb_enable(struct tegra_output *output)
> >         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
> >         tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
> >
> > +       rgb->enabled = true;
> > +
> >         return 0;
> >  }
> >
> > @@ -130,6 +136,9 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
> >         struct tegra_rgb *rgb = to_rgb(output);
> >         unsigned long value;
> >
> > +       if (!rgb->enabled)
> > +               return 0;
> > +
> >         value = tegra_dc_readl(rgb->dc, DC_CMD_DISPLAY_POWER_CONTROL);
> >         value &= ~(PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
> >                    PW4_ENABLE | PM0_ENABLE | PM1_ENABLE);
> > @@ -144,6 +153,8 @@ static int tegra_output_rgb_disable(struct tegra_output *output)
> >
> >         tegra_dc_write_regs(rgb->dc, rgb_disable, ARRAY_SIZE(rgb_disable));
> >
> > +       rgb->enabled = false;
> > +
> >         return 0;
> >  }
> >
> 
> Wouldn't it make more sense to make "enabled" and int that counts how
> many times tegra_output_rgb_enable has been called? That way you can
> have tegra_output_rgb_disable only really disable the display once the
> same amount of disables have been performed...

I don't think that will work. Calls to tegra_output_rgb_enable/disable()
and the .enable/disable for any other outputs aren't guaranteed to be
balanced. Therefore reference counting the enables and disables will
break things.

Thinking about it, maybe the current interface to outputs isn't the best
solution. It's simple and with the guards in place it works pretty well.
Perhaps if I can find some spare time I'll investigate whether something
a little cleaner can be done.

Thierry
Thierry Reding Feb. 12, 2014, 10:18 a.m. UTC | #4
On Tue, Feb 11, 2014 at 09:12:27PM +0400, Dmitry Osipenko wrote:
> Add guard to check whether RGB output is already enabled in the way it's
> done for HDMI output. Fixes possible hang on trying to disable output twice
> (first time during driver probe and second on fb registering).
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/rgb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Applied, thanks!

Thierry
diff mbox

Patch

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 338f7f6..0266fb4 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -15,6 +15,7 @@ 
 struct tegra_rgb {
 	struct tegra_output output;
 	struct tegra_dc *dc;
+	bool enabled;
 
 	struct clk *clk_parent;
 	struct clk *clk;
@@ -89,6 +90,9 @@  static int tegra_output_rgb_enable(struct tegra_output *output)
 	struct tegra_rgb *rgb = to_rgb(output);
 	unsigned long value;
 
+	if (rgb->enabled)
+		return 0;
+
 	tegra_dc_write_regs(rgb->dc, rgb_enable, ARRAY_SIZE(rgb_enable));
 
 	value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
@@ -122,6 +126,8 @@  static int tegra_output_rgb_enable(struct tegra_output *output)
 	tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
 	tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
 
+	rgb->enabled = true;
+
 	return 0;
 }
 
@@ -130,6 +136,9 @@  static int tegra_output_rgb_disable(struct tegra_output *output)
 	struct tegra_rgb *rgb = to_rgb(output);
 	unsigned long value;
 
+	if (!rgb->enabled)
+		return 0;
+
 	value = tegra_dc_readl(rgb->dc, DC_CMD_DISPLAY_POWER_CONTROL);
 	value &= ~(PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
 		   PW4_ENABLE | PM0_ENABLE | PM1_ENABLE);
@@ -144,6 +153,8 @@  static int tegra_output_rgb_disable(struct tegra_output *output)
 
 	tegra_dc_write_regs(rgb->dc, rgb_disable, ARRAY_SIZE(rgb_disable));
 
+	rgb->enabled = false;
+
 	return 0;
 }