[PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Liviu Dudau]

Hello,

While updating the HDLCD driver to support atomic modesetting I've encountered crashes
when using it with the tda998x driver because the later doesn't support the atomic
helper functions. While going through the code testing I've noticed an unbalanced
.unbind missing drm_connector_unregister() and updated the pixel clock support for
TDA19988.

These patches are to be applied on top of David Airlie's drm-next. I've used commit
816d2206f0f9 as that includes Russell's cleanup for tda998x that has gone into v4.4-rc1.

Best regards,
Liviu

Liviu Dudau (3):
  drm/i2c: tda998x: Unregister the connector in the unbind function.
  drm/i2c: tda998x: Increase the supported dotclock frequency to 165MHz for TDA19988.
  drm/i2c: tda998x: Add support for atomic modesetting.

 drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.6.0

[PATCH 1/3] drm/i2c: tda998x: Unregister the connector in the unbind function.

[Liviu Dudau]

tda998x uses drm_connector_register() in the .bind function that
needs to be balanced with a drm_connector_unregister() in the .unbind.
Otherwise dangling sysfs entries are left behind and future rebinds
will fail.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 896b6aa..cdbd83b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1472,6 +1472,7 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
 
+	drm_connector_unregister(&priv->connector);
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
 	tda998x_destroy(priv);
-- 
2.6.0

[PATCH 2/3] drm/i2c: tda998x: Increase the supported dotclock frequency to 165MHz for TDA19988.

[Liviu Dudau]

Spec sheet states that the TDA19988 supports up to 165MHz dotclock speeds.
Without this change modes higher than 1080p are un-attainable.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 5 +++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index cdbd83b..a9c8ee8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -878,7 +878,10 @@ tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
 static int tda998x_connector_mode_valid(struct drm_connector *connector,
 					struct drm_display_mode *mode)
 {
-	if (mode->clock > 150000)
+	/* TDA19988 dotclock can go up to 165MHz */
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000))
 		return MODE_CLOCK_HIGH;
 	if (mode->htotal >= BIT(13))
 		return MODE_BAD_HVALUE;
-- 
2.6.0

[PATCH 3/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Liviu Dudau]

When used with a DRIVER_ATOMIC enabled CRTC driver, the tda998x
will cause crashes due to missing atomic operations. Fill the
drm_connector_funcs struct with the atomic versions of the required
functions and add the atomic modeset specific callbacks.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a9c8ee8..5c94cea 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -22,6 +22,7 @@
 #include <sound/asoundef.h>
 
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
@@ -1395,10 +1396,13 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs tda998x_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = tda998x_connector_detect,
 	.destroy = tda998x_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
-- 
2.6.0

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Russell King - ARM Linux]

On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> While going through the code testing I've noticed an unbalanced
> .unbind missing drm_connector_unregister()

That actually doesn't matter, as DRM automatically tears them down anyway,
so this isn't an urgent change.  However, it's good practice to do so.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Liviu Dudau]

On Wed, Nov 11, 2015 at 05:51:52PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> > While going through the code testing I've noticed an unbalanced
> > .unbind missing drm_connector_unregister()
> 
> That actually doesn't matter, as DRM automatically tears them down anyway,
> so this isn't an urgent change.  However, it's good practice to do so.

It looks like it doesn't, or at least not if the error code is -EPROBE_DEFER.
On Juno, where the clocks are provided by SCPI and the load order is not
guaranteed, the first bind will fail with -EPROBE_DEFER but the sysfs entry
is not cleaned up, so on the next attempt the drm_connector_register() call
will fail.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Liviu Dudau]

On Wed, Nov 11, 2015 at 05:57:18PM +0000, Liviu Dudau wrote:
> On Wed, Nov 11, 2015 at 05:51:52PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> > > While going through the code testing I've noticed an unbalanced
> > > .unbind missing drm_connector_unregister()
> > 
> > That actually doesn't matter, as DRM automatically tears them down anyway,
> > so this isn't an urgent change.  However, it's good practice to do so.
> 
> It looks like it doesn't, or at least not if the error code is -EPROBE_DEFER.
> On Juno, where the clocks are provided by SCPI and the load order is not
> guaranteed, the first bind will fail with -EPROBE_DEFER but the sysfs entry
> is not cleaned up, so on the next attempt the drm_connector_register() call
> will fail.
> 
> Best regards,
> Liviu

Gentle ping. Russell, are you happy with this patchset? If so, would you mind
giving me your Acks?

Many thanks,
Liviu

> 
> > 
> > -- 
> > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> > according to speedtest.net.
> > 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Russell King - ARM Linux]

On Fri, Nov 20, 2015 at 02:24:04PM +0000, Liviu Dudau wrote:
> On Wed, Nov 11, 2015 at 05:57:18PM +0000, Liviu Dudau wrote:
> > On Wed, Nov 11, 2015 at 05:51:52PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> > > > While going through the code testing I've noticed an unbalanced
> > > > .unbind missing drm_connector_unregister()
> > > 
> > > That actually doesn't matter, as DRM automatically tears them down anyway,
> > > so this isn't an urgent change.  However, it's good practice to do so.
> > 
> > It looks like it doesn't, or at least not if the error code is -EPROBE_DEFER.
> > On Juno, where the clocks are provided by SCPI and the load order is not
> > guaranteed, the first bind will fail with -EPROBE_DEFER but the sysfs entry
> > is not cleaned up, so on the next attempt the drm_connector_register() call
> > will fail.
> > 
> > Best regards,
> > Liviu
> 
> Gentle ping. Russell, are you happy with this patchset? If so, would you mind
> giving me your Acks?

As I'm the maintainer for the driver, I'll merge it, thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Liviu Dudau]

On Fri, Nov 20, 2015 at 04:32:59PM +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 20, 2015 at 02:24:04PM +0000, Liviu Dudau wrote:
> > On Wed, Nov 11, 2015 at 05:57:18PM +0000, Liviu Dudau wrote:
> > > On Wed, Nov 11, 2015 at 05:51:52PM +0000, Russell King - ARM Linux wrote:
> > > > On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> > > > > While going through the code testing I've noticed an unbalanced
> > > > > .unbind missing drm_connector_unregister()
> > > > 
> > > > That actually doesn't matter, as DRM automatically tears them down anyway,
> > > > so this isn't an urgent change.  However, it's good practice to do so.
> > > 
> > > It looks like it doesn't, or at least not if the error code is -EPROBE_DEFER.
> > > On Juno, where the clocks are provided by SCPI and the load order is not
> > > guaranteed, the first bind will fail with -EPROBE_DEFER but the sysfs entry
> > > is not cleaned up, so on the next attempt the drm_connector_register() call
> > > will fail.
> > > 
> > > Best regards,
> > > Liviu
> > 
> > Gentle ping. Russell, are you happy with this patchset? If so, would you mind
> > giving me your Acks?
> 
> As I'm the maintainer for the driver, I'll merge it, thanks.

Cheers!

Do I need to do anything?

Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 0/3] drm/i2c: tda998x: Add support for atomic modesetting.

[Russell King - ARM Linux]

On Fri, Nov 20, 2015 at 04:44:55PM +0000, Liviu Dudau wrote:
> On Fri, Nov 20, 2015 at 04:32:59PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Nov 20, 2015 at 02:24:04PM +0000, Liviu Dudau wrote:
> > > On Wed, Nov 11, 2015 at 05:57:18PM +0000, Liviu Dudau wrote:
> > > > On Wed, Nov 11, 2015 at 05:51:52PM +0000, Russell King - ARM Linux wrote:
> > > > > On Wed, Nov 11, 2015 at 03:34:32PM +0000, Liviu Dudau wrote:
> > > > > > While going through the code testing I've noticed an unbalanced
> > > > > > .unbind missing drm_connector_unregister()
> > > > > 
> > > > > That actually doesn't matter, as DRM automatically tears them down anyway,
> > > > > so this isn't an urgent change.  However, it's good practice to do so.
> > > > 
> > > > It looks like it doesn't, or at least not if the error code is -EPROBE_DEFER.
> > > > On Juno, where the clocks are provided by SCPI and the load order is not
> > > > guaranteed, the first bind will fail with -EPROBE_DEFER but the sysfs entry
> > > > is not cleaned up, so on the next attempt the drm_connector_register() call
> > > > will fail.
> > > > 
> > > > Best regards,
> > > > Liviu
> > > 
> > > Gentle ping. Russell, are you happy with this patchset? If so, would you mind
> > > giving me your Acks?
> > 
> > As I'm the maintainer for the driver, I'll merge it, thanks.
> 
> Cheers!
> 
> Do I need to do anything?

The easy way to ensure that it doesn't get forgotten is to put it in my
patch system, just like ARM patches do.  It'll then nag me each time I
look at it (and also means I don't have to save it out, copy it across
to the machine with the git tree on it, and then apply it there...)

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.