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(-)
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);
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;
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)
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.
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.
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.
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.
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.
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.
dri-devel@lists.freedesktop.org
-
Liviu Dudau -
Russell King - ARM Linux