[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[Chen-Yu Tsai]

The sun4i DRM driver counts the number of endpoints it found and
registers the whole DRM pipeline if any endpoints are found.

However, if the TCON and its child endpoints (LCD panels, TV encoder,
HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
don't have any usable CRTCs, and the display pipeline is incomplete
and useless. The whole DRM display pipeline should only be registered
and enabled if there are proper outputs available.

The debug message "Queued %d outputs on pipeline %d\n" is also telling.

This patch makes the driver only count enabled TCON endpoints. If
none are found, the DRM pipeline is not used. This avoids screwing
up the simple framebuffer provided by the bootloader in cases where
we aren't able to support the display with the DRM subsystem, due
to lack of panel or bridge drivers, or just lack of progress.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---

Hi Maxime,

This avoids DRM screwing up simplefb on my SinA31s, which does not
have the display pipeline enabled in its dts file. But the display
engine and backend are already enabled in the dtsi.

I think this is a better and proper (for the driver) fix. The
alternative would be to disable the display-engine node in the dts
by default. Last time I asked you wanted to have them enabled by
default?

It may also be possible to push the check further down, and check
against panel and encoder endpoints, but I think that complicates
things. The TCON is a necessary part of the output.

ChenYu

---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index c3b21865443e..3603f34901b6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -243,9 +243,12 @@ static int sun4i_drv_add_endpoints(struct device *dev,
 		DRM_DEBUG_DRIVER("Adding component %s\n",
 				 of_node_full_name(node));
 		component_match_add(dev, match, compare_of, node);
-		count++;
 	}
 
+	/* Only count the tcon as an output */
+	if (sun4i_drv_node_is_tcon(node))
+		count++;
+
 	/* Inputs are listed first, then outputs */
 	port = of_graph_get_port_by_id(node, 1);
 	if (!port) {
-- 
2.10.2

[PATCH] drm/sun4i: tcon: Initialize regmap after enabling bus clocks

[Chen-Yu Tsai]

If we attempt to read/write the TCON registers before the bus clock
is enabled, those accesses get ignored.

In practice this almost never occurs because U-boot had already enabled
the bus clock as part of its firmware provided framebuffer (simplefb).

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---

I was looking around the DRM driver and noticed this sequence was off.

---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c6afb2448655..8c2db65ea229 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -506,16 +506,16 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	ret = sun4i_tcon_init_regmap(dev, tcon);
+	ret = sun4i_tcon_init_clocks(dev, tcon);
 	if (ret) {
-		dev_err(dev, "Couldn't init our TCON regmap\n");
+		dev_err(dev, "Couldn't init our TCON clocks\n");
 		goto err_assert_reset;
 	}
 
-	ret = sun4i_tcon_init_clocks(dev, tcon);
+	ret = sun4i_tcon_init_regmap(dev, tcon);
 	if (ret) {
-		dev_err(dev, "Couldn't init our TCON clocks\n");
-		goto err_assert_reset;
+		dev_err(dev, "Couldn't init our TCON regmap\n");
+		goto err_free_clocks;
 	}
 
 	ret = sun4i_tcon_init_irq(dev, tcon);
-- 
2.10.2

[PATCH] drm/sun4i: tcon: Initialize regmap after enabling bus clocks

[Maxime Ripard]

On Wed, Nov 16, 2016 at 05:37:32PM +0800, Chen-Yu Tsai wrote:
> If we attempt to read/write the TCON registers before the bus clock
> is enabled, those accesses get ignored.
> 
> In practice this almost never occurs because U-boot had already enabled
> the bus clock as part of its firmware provided framebuffer (simplefb).
> 
> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> 
> I was looking around the DRM driver and noticed this sequence was off.
> 
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c6afb2448655..8c2db65ea229 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -506,16 +506,16 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>  
> -	ret = sun4i_tcon_init_regmap(dev, tcon);
> +	ret = sun4i_tcon_init_clocks(dev, tcon);
>  	if (ret) {
> -		dev_err(dev, "Couldn't init our TCON regmap\n");
> +		dev_err(dev, "Couldn't init our TCON clocks\n");
>  		goto err_assert_reset;
>  	}
>  
> -	ret = sun4i_tcon_init_clocks(dev, tcon);
> +	ret = sun4i_tcon_init_regmap(dev, tcon);
>  	if (ret) {
> -		dev_err(dev, "Couldn't init our TCON clocks\n");
> -		goto err_assert_reset;
> +		dev_err(dev, "Couldn't init our TCON regmap\n");
> +		goto err_free_clocks;
>  	}

That won't work either. sun4i_tcon_init_clocks requires the regmap to
be enabled before it calls sun4i_dclk_create.

Maybe we should just move that call outside of sun4i_tcon_init_clocks
and put it directly into the bind then.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161117/ac4950fb/attachment.sig>

[PATCH] drm/sun4i: tcon: Initialize regmap after enabling bus clocks

[Chen-Yu Tsai]

On Fri, Nov 18, 2016 at 3:05 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Nov 16, 2016 at 05:37:32PM +0800, Chen-Yu Tsai wrote:
>> If we attempt to read/write the TCON registers before the bus clock
>> is enabled, those accesses get ignored.
>>
>> In practice this almost never occurs because U-boot had already enabled
>> the bus clock as part of its firmware provided framebuffer (simplefb).
>>
>> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>
>> I was looking around the DRM driver and noticed this sequence was off.
>>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index c6afb2448655..8c2db65ea229 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -506,16 +506,16 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>>               return ret;
>>       }
>>
>> -     ret = sun4i_tcon_init_regmap(dev, tcon);
>> +     ret = sun4i_tcon_init_clocks(dev, tcon);
>>       if (ret) {
>> -             dev_err(dev, "Couldn't init our TCON regmap\n");
>> +             dev_err(dev, "Couldn't init our TCON clocks\n");
>>               goto err_assert_reset;
>>       }
>>
>> -     ret = sun4i_tcon_init_clocks(dev, tcon);
>> +     ret = sun4i_tcon_init_regmap(dev, tcon);
>>       if (ret) {
>> -             dev_err(dev, "Couldn't init our TCON clocks\n");
>> -             goto err_assert_reset;
>> +             dev_err(dev, "Couldn't init our TCON regmap\n");
>> +             goto err_free_clocks;
>>       }
>
> That won't work either. sun4i_tcon_init_clocks requires the regmap to
> be enabled before it calls sun4i_dclk_create.
>
> Maybe we should just move that call outside of sun4i_tcon_init_clocks
> and put it directly into the bind then.

That makes sense. I'll send a v2.

ChenYu

[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[Maxime Ripard]

On Wed, Nov 16, 2016 at 05:37:31PM +0800, Chen-Yu Tsai wrote:
> The sun4i DRM driver counts the number of endpoints it found and
> registers the whole DRM pipeline if any endpoints are found.
> 
> However, if the TCON and its child endpoints (LCD panels, TV encoder,
> HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
> don't have any usable CRTCs, and the display pipeline is incomplete
> and useless.

If some node set as available is not probed, then yes, it does, but
I'm not really sure how it's a problem. Quite the opposite actually.

> The whole DRM display pipeline should only be registered and enabled
> if there are proper outputs available.

Unless I've misunderstood what you're saying, we could have the
writeback for example that would just need the display engine.

> The debug message "Queued %d outputs on pipeline %d\n" is also telling.
> 
> This patch makes the driver only count enabled TCON endpoints. If
> none are found, the DRM pipeline is not used. This avoids screwing
> up the simple framebuffer provided by the bootloader in cases where
> we aren't able to support the display with the DRM subsystem, due
> to lack of panel or bridge drivers, or just lack of progress.

The framebuffer is removed only at bind time, which means that all the
drivers have probed already. Lack of progress isn't an issue here,
since the node simply won't be there, and we wouldn't have it in the
component lists. And lack of drivers shouldn't be an issue either,
since in order for bind to be called, all the drivers would have
gone through their probe.

So I'm not really sure what it fixes.
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161117/9e378540/attachment.sig>

[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[Chen-Yu Tsai]

On Fri, Nov 18, 2016 at 3:02 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Nov 16, 2016 at 05:37:31PM +0800, Chen-Yu Tsai wrote:
>> The sun4i DRM driver counts the number of endpoints it found and
>> registers the whole DRM pipeline if any endpoints are found.
>>
>> However, if the TCON and its child endpoints (LCD panels, TV encoder,
>> HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
>> don't have any usable CRTCs, and the display pipeline is incomplete
>> and useless.
>
> If some node set as available is not probed, then yes, it does, but
> I'm not really sure how it's a problem. Quite the opposite actually.

Actually the problem occurs when the TCON is _not_ available, but
the other endpoints preceding it are.

>> The whole DRM display pipeline should only be registered and enabled
>> if there are proper outputs available.
>
> Unless I've misunderstood what you're saying, we could have the
> writeback for example that would just need the display engine.

Yeah, I guess that complicates things...

>> The debug message "Queued %d outputs on pipeline %d\n" is also telling.
>>
>> This patch makes the driver only count enabled TCON endpoints. If
>> none are found, the DRM pipeline is not used. This avoids screwing
>> up the simple framebuffer provided by the bootloader in cases where
>> we aren't able to support the display with the DRM subsystem, due
>> to lack of panel or bridge drivers, or just lack of progress.
>
> The framebuffer is removed only at bind time, which means that all the
> drivers have probed already. Lack of progress isn't an issue here,
> since the node simply won't be there, and we wouldn't have it in the
> component lists. And lack of drivers shouldn't be an issue either,
> since in order for bind to be called, all the drivers would have
> gone through their probe.
>
> So I'm not really sure what it fixes.

To recap, on sun6i I had enabled the display engine node by default
in the dtsi, along with the backend and drc. The tcon is disabled
by default, so it doesn't get added to the list of components.
The available components get probed, binded, and simplefb gets
pushed out.

I suppose disabling the display engine by default would be better?
At least simplefb still works.

Regards
ChenYu

> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[Maxime Ripard]

Hi,

On Fri, Nov 18, 2016 at 10:22:40AM +0800, Chen-Yu Tsai wrote:
> On Fri, Nov 18, 2016 at 3:02 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Nov 16, 2016 at 05:37:31PM +0800, Chen-Yu Tsai wrote:
> >> The sun4i DRM driver counts the number of endpoints it found and
> >> registers the whole DRM pipeline if any endpoints are found.
> >>
> >> However, if the TCON and its child endpoints (LCD panels, TV encoder,
> >> HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
> >> don't have any usable CRTCs, and the display pipeline is incomplete
> >> and useless.
> >
> > If some node set as available is not probed, then yes, it does, but
> > I'm not really sure how it's a problem. Quite the opposite actually.
> 
> Actually the problem occurs when the TCON is _not_ available, but
> the other endpoints preceding it are.

By preceding, you mean the display engine or the HDMI or TV encoders?

> >> The debug message "Queued %d outputs on pipeline %d\n" is also telling.
> >>
> >> This patch makes the driver only count enabled TCON endpoints. If
> >> none are found, the DRM pipeline is not used. This avoids screwing
> >> up the simple framebuffer provided by the bootloader in cases where
> >> we aren't able to support the display with the DRM subsystem, due
> >> to lack of panel or bridge drivers, or just lack of progress.
> >
> > The framebuffer is removed only at bind time, which means that all the
> > drivers have probed already. Lack of progress isn't an issue here,
> > since the node simply won't be there, and we wouldn't have it in the
> > component lists. And lack of drivers shouldn't be an issue either,
> > since in order for bind to be called, all the drivers would have
> > gone through their probe.
> >
> > So I'm not really sure what it fixes.
> 
> To recap, on sun6i I had enabled the display engine node by default
> in the dtsi, along with the backend and drc. The tcon is disabled
> by default, so it doesn't get added to the list of components.
> The available components get probed, binded, and simplefb gets
> pushed out.
> 
> I suppose disabling the display engine by default would be better?
> At least simplefb still works.

Yep, that works for me.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161122/65608cd9/attachment.sig>

[PATCH] drm/sun4i: Only count TCON endpoints as valid outputs

[Chen-Yu Tsai]

On Tue, Nov 22, 2016 at 11:37 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Fri, Nov 18, 2016 at 10:22:40AM +0800, Chen-Yu Tsai wrote:
>> On Fri, Nov 18, 2016 at 3:02 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Wed, Nov 16, 2016 at 05:37:31PM +0800, Chen-Yu Tsai wrote:
>> >> The sun4i DRM driver counts the number of endpoints it found and
>> >> registers the whole DRM pipeline if any endpoints are found.
>> >>
>> >> However, if the TCON and its child endpoints (LCD panels, TV encoder,
>> >> HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
>> >> don't have any usable CRTCs, and the display pipeline is incomplete
>> >> and useless.
>> >
>> > If some node set as available is not probed, then yes, it does, but
>> > I'm not really sure how it's a problem. Quite the opposite actually.
>>
>> Actually the problem occurs when the TCON is _not_ available, but
>> the other endpoints preceding it are.
>
> By preceding, you mean the display engine or the HDMI or TV encoders?

The display engine.

>> >> The debug message "Queued %d outputs on pipeline %d\n" is also telling.
>> >>
>> >> This patch makes the driver only count enabled TCON endpoints. If
>> >> none are found, the DRM pipeline is not used. This avoids screwing
>> >> up the simple framebuffer provided by the bootloader in cases where
>> >> we aren't able to support the display with the DRM subsystem, due
>> >> to lack of panel or bridge drivers, or just lack of progress.
>> >
>> > The framebuffer is removed only at bind time, which means that all the
>> > drivers have probed already. Lack of progress isn't an issue here,
>> > since the node simply won't be there, and we wouldn't have it in the
>> > component lists. And lack of drivers shouldn't be an issue either,
>> > since in order for bind to be called, all the drivers would have
>> > gone through their probe.
>> >
>> > So I'm not really sure what it fixes.
>>
>> To recap, on sun6i I had enabled the display engine node by default
>> in the dtsi, along with the backend and drc. The tcon is disabled
>> by default, so it doesn't get added to the list of components.
>> The available components get probed, binded, and simplefb gets
>> pushed out.
>>
>> I suppose disabling the display engine by default would be better?
>> At least simplefb still works.
>
> Yep, that works for me.

OK. I'll send out a patch.

ChenYu