* Re: [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
2016-01-16 20:17 [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper Jyri Sarha
@ 2016-01-18 9:52 ` Liviu.Dudau
2016-01-18 11:15 ` Russell King - ARM Linux
2016-02-25 10:09 ` Jyri Sarha
2016-03-07 21:47 ` Jyri Sarha
2 siblings, 1 reply; 7+ messages in thread
From: Liviu.Dudau @ 2016-01-18 9:52 UTC (permalink / raw)
To: Jyri Sarha; +Cc: dri-devel, tomi.valkeinen, rmk+kernel, linux-omap
On Sat, Jan 16, 2016 at 10:17:54PM +0200, Jyri Sarha wrote:
> Choose between atomic or non atomic connector dpms helper. If tda998x
> is connected to a drm driver that does not support atomic modeset
> calling drm_atomic_helper_connector_dpms() causes a crash when the
> connectors atomic state is not initialized. The patch implements a
> driver specific connector dpms helper that calls
> drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> and otherwise it calls the legacy drm_helper_connector_dpms().
>
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
Looks much cleaner to me.
Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
>
> Ok, so this is the second approach to solve this issue. The first
> attempt can be found here [1] with the follow from Liviu Dudau that
> suggested this approach.
>
> It just makes me wonder if drm_atomic_helper_connector_dpms() should
> call the legacy callback automatically if DRIVER_ATOMIC is not set or
> at least bail out gracefully with an error message. Then again it may
> be overkill if the tda998x is the only driver that need to support
> both situations.
I would guess that in general it makes sense for the core code to do what
you suggest, but the idea is to try to encourage the drivers to convert
to atomic ops rather than give them too many crutches. And the tda998x will
need this for a short period of time until tilcdc converts as well.
Best regards,
Liviu
>
> Best regards,
> Jyri
>
> [1] http://www.spinics.net/lists/dri-devel/msg98514.html
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..bb7d507 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
> drm_connector_cleanup(connector);
> }
>
> +static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> +{
> + if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> + return drm_atomic_helper_connector_dpms(connector, mode);
> + else
> + return drm_helper_connector_dpms(connector, mode);
> +}
> +
> static const struct drm_connector_funcs tda998x_connector_funcs = {
> - .dpms = drm_atomic_helper_connector_dpms,
> + .dpms = tda998x_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> .detect = tda998x_connector_detect,
> --
> 1.9.1
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
2016-01-18 9:52 ` Liviu.Dudau
@ 2016-01-18 11:15 ` Russell King - ARM Linux
0 siblings, 0 replies; 7+ messages in thread
From: Russell King - ARM Linux @ 2016-01-18 11:15 UTC (permalink / raw)
To: Liviu.Dudau; +Cc: dri-devel, tomi.valkeinen, Jyri Sarha, linux-omap
On Mon, Jan 18, 2016 at 09:52:27AM +0000, Liviu.Dudau@arm.com wrote:
> On Sat, Jan 16, 2016 at 10:17:54PM +0200, Jyri Sarha wrote:
> > It just makes me wonder if drm_atomic_helper_connector_dpms() should
> > call the legacy callback automatically if DRIVER_ATOMIC is not set or
> > at least bail out gracefully with an error message. Then again it may
> > be overkill if the tda998x is the only driver that need to support
> > both situations.
>
> I would guess that in general it makes sense for the core code to do what
> you suggest, but the idea is to try to encourage the drivers to convert
> to atomic ops rather than give them too many crutches. And the tda998x will
> need this for a short period of time until tilcdc converts as well.
It's not just tilcdc that this driver is used by.
I have no plans at present to convert armada to atomic mode setting,
although I occasionally push it (I hope) towards that goal. This
isn't going to happen quickly, because I've other areas that need
work too.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
2016-01-16 20:17 [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper Jyri Sarha
2016-01-18 9:52 ` Liviu.Dudau
@ 2016-02-25 10:09 ` Jyri Sarha
2016-02-25 10:20 ` Liviu.Dudau
2016-03-07 21:47 ` Jyri Sarha
2 siblings, 1 reply; 7+ messages in thread
From: Jyri Sarha @ 2016-02-25 10:09 UTC (permalink / raw)
To: dri-devel, mk+kernel; +Cc: Liviu.Dudau
Hi,
Based on discussion around this patch:
https://lists.freedesktop.org/archives/dri-devel/2016-February/100685.html
I think the patch below should be applied to tda988x development branch.
Would you take it or do you prefer some other approach?
Best regards,
Jyri
On 01/16/16 22:17, Jyri Sarha wrote:
> Choose between atomic or non atomic connector dpms helper. If tda998x
> is connected to a drm driver that does not support atomic modeset
> calling drm_atomic_helper_connector_dpms() causes a crash when the
> connectors atomic state is not initialized. The patch implements a
> driver specific connector dpms helper that calls
> drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> and otherwise it calls the legacy drm_helper_connector_dpms().
>
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>
> Ok, so this is the second approach to solve this issue. The first
> attempt can be found here [1] with the follow from Liviu Dudau that
> suggested this approach.
>
> It just makes me wonder if drm_atomic_helper_connector_dpms() should
> call the legacy callback automatically if DRIVER_ATOMIC is not set or
> at least bail out gracefully with an error message. Then again it may
> be overkill if the tda998x is the only driver that need to support
> both situations.
>
> Best regards,
> Jyri
>
> [1] http://www.spinics.net/lists/dri-devel/msg98514.html
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..bb7d507 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
> drm_connector_cleanup(connector);
> }
>
> +static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> +{
> + if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> + return drm_atomic_helper_connector_dpms(connector, mode);
> + else
> + return drm_helper_connector_dpms(connector, mode);
> +}
> +
> static const struct drm_connector_funcs tda998x_connector_funcs = {
> - .dpms = drm_atomic_helper_connector_dpms,
> + .dpms = tda998x_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> .detect = tda998x_connector_detect,
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
2016-02-25 10:09 ` Jyri Sarha
@ 2016-02-25 10:20 ` Liviu.Dudau
0 siblings, 0 replies; 7+ messages in thread
From: Liviu.Dudau @ 2016-02-25 10:20 UTC (permalink / raw)
To: Jyri Sarha; +Cc: mk+kernel, dri-devel
On Thu, Feb 25, 2016 at 12:09:39PM +0200, Jyri Sarha wrote:
> Hi,
> Based on discussion around this patch:
>
> https://lists.freedesktop.org/archives/dri-devel/2016-February/100685.html
>
> I think the patch below should be applied to tda988x development branch.
> Would you take it or do you prefer some other approach?
You should send it to David Airlie after you get an Ack from Russell.
Best regards,
Liviu
>
> Best regards,
> Jyri
>
> On 01/16/16 22:17, Jyri Sarha wrote:
> >Choose between atomic or non atomic connector dpms helper. If tda998x
> >is connected to a drm driver that does not support atomic modeset
> >calling drm_atomic_helper_connector_dpms() causes a crash when the
> >connectors atomic state is not initialized. The patch implements a
> >driver specific connector dpms helper that calls
> >drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> >and otherwise it calls the legacy drm_helper_connector_dpms().
> >
> >Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> >modesetting").
> >
> >Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >---
> >
> >Ok, so this is the second approach to solve this issue. The first
> >attempt can be found here [1] with the follow from Liviu Dudau that
> >suggested this approach.
> >
> >It just makes me wonder if drm_atomic_helper_connector_dpms() should
> >call the legacy callback automatically if DRIVER_ATOMIC is not set or
> >at least bail out gracefully with an error message. Then again it may
> >be overkill if the tda998x is the only driver that need to support
> >both situations.
> >
> >Best regards,
> >Jyri
> >
> >[1] http://www.spinics.net/lists/dri-devel/msg98514.html
> >
> > drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> >index 012d36d..bb7d507 100644
> >--- a/drivers/gpu/drm/i2c/tda998x_drv.c
> >+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >@@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
> > drm_connector_cleanup(connector);
> > }
> >
> >+static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> >+{
> >+ if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> >+ return drm_atomic_helper_connector_dpms(connector, mode);
> >+ else
> >+ return drm_helper_connector_dpms(connector, mode);
> >+}
> >+
> > static const struct drm_connector_funcs tda998x_connector_funcs = {
> >- .dpms = drm_atomic_helper_connector_dpms,
> >+ .dpms = tda998x_connector_dpms,
> > .reset = drm_atomic_helper_connector_reset,
> > .fill_modes = drm_helper_probe_single_connector_modes,
> > .detect = tda998x_connector_detect,
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
2016-01-16 20:17 [PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper Jyri Sarha
2016-01-18 9:52 ` Liviu.Dudau
2016-02-25 10:09 ` Jyri Sarha
@ 2016-03-07 21:47 ` Jyri Sarha
2016-03-08 1:16 ` Dave Airlie
2 siblings, 1 reply; 7+ messages in thread
From: Jyri Sarha @ 2016-03-07 21:47 UTC (permalink / raw)
To: dri-devel, airlied, linux-omap; +Cc: rmk+kernel, tomi.valkeinen, Liviu.Dudau
Hi,
Based on discussion around this patch:
https://lists.freedesktop.org/archives/dri-devel/2016-February/100685.html
I think the patch below should get into queued for v4.5 fixes as the
Beaglebone-Black display is currently broken in linux-master and
probably Armada is broken too. What should I do to get it there?
Best regards,
Jyri
On 01/16/16 22:17, Jyri Sarha wrote:
> Choose between atomic or non atomic connector dpms helper. If tda998x
> is connected to a drm driver that does not support atomic modeset
> calling drm_atomic_helper_connector_dpms() causes a crash when the
> connectors atomic state is not initialized. The patch implements a
> driver specific connector dpms helper that calls
> drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> and otherwise it calls the legacy drm_helper_connector_dpms().
>
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>
> Ok, so this is the second approach to solve this issue. The first
> attempt can be found here [1] with the follow from Liviu Dudau that
> suggested this approach.
>
> It just makes me wonder if drm_atomic_helper_connector_dpms() should
> call the legacy callback automatically if DRIVER_ATOMIC is not set or
> at least bail out gracefully with an error message. Then again it may
> be overkill if the tda998x is the only driver that need to support
> both situations.
>
> Best regards,
> Jyri
>
> [1] http://www.spinics.net/lists/dri-devel/msg98514.html
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..bb7d507 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
> drm_connector_cleanup(connector);
> }
>
> +static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> +{
> + if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> + return drm_atomic_helper_connector_dpms(connector, mode);
> + else
> + return drm_helper_connector_dpms(connector, mode);
> +}
> +
> static const struct drm_connector_funcs tda998x_connector_funcs = {
> - .dpms = drm_atomic_helper_connector_dpms,
> + .dpms = tda998x_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> .detect = tda998x_connector_detect,
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread