diff mbox

[RFC] drm/i2c: tda998x: Only set atomic funcs if bound to atomic drm driver

Message ID 1452862526-32176-1-git-send-email-jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Jan. 15, 2016, 12:55 p.m. UTC
Only set atomic connector callbacks if the tda998x driver is bound to
a drm driver that supports atomic modesetting. At least
drm_crtc_helper_set_config() calls connectors dpms callback without
knowing that it may assume atomic modeset support. Calling
drm_atomic_helper_connector_dpms() causes a crash with a driver that
does not have atomic state initialized.

Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
modesetting").

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
I am not sure if this is the best way to fix the issue, but after
9736e988d328 Beaglebone-Black HDMI is totally broken and this fixes
the issue. I am currently working on atomic modesetting for tilcdc,
but there is no way it makes it to 4.5 release.

Best regards,
Jyri

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

Comments

Liviu Dudau Jan. 15, 2016, 2:09 p.m. UTC | #1
On Fri, Jan 15, 2016 at 02:55:26PM +0200, Jyri Sarha wrote:
> Only set atomic connector callbacks if the tda998x driver is bound to
> a drm driver that supports atomic modesetting. At least
> drm_crtc_helper_set_config() calls connectors dpms callback without
> knowing that it may assume atomic modeset support. Calling
> drm_atomic_helper_connector_dpms() causes a crash with a driver that
> does not have atomic state initialized.

Hi Jyri,

> 
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").

First, sorry for the breakage, I did not had any other board to test the
change on.

> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
> I am not sure if this is the best way to fix the issue, but after
> 9736e988d328 Beaglebone-Black HDMI is totally broken and this fixes
> the issue. I am currently working on atomic modesetting for tilcdc,
> but there is no way it makes it to 4.5 release.

Yes, I'm not a big fan of the change either. I would've thought that
you only need to update the .dpms pointer, all other hooks added are
specific to atomic operations (right?). Would it not be simpler to have
a tda998x_connector_dpms() function that calls the appropriate .dpms
function based on the feature check?

Best regards,
Liviu

> 
> Best regards,
> Jyri
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..67d1408 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,7 +1382,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
>  	drm_connector_cleanup(connector);
>  }
>  
> -static const struct drm_connector_funcs tda998x_connector_funcs = {
> +static const struct drm_connector_funcs tda998x_connector_atomic_funcs = {
>  	.dpms = drm_atomic_helper_connector_dpms,
>  	.reset = drm_atomic_helper_connector_reset,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -1392,15 +1392,28 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static const struct drm_connector_funcs tda998x_connector_funcs = {
> +	.dpms = drm_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = tda998x_connector_detect,
> +	.destroy = tda998x_connector_destroy,
> +};
> +
>  static int tda998x_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct tda998x_encoder_params *params = dev->platform_data;
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct drm_device *drm = data;
>  	struct tda998x_priv *priv;
> +	const struct drm_connector_funcs *connector_funcs;
>  	u32 crtcs = 0;
>  	int ret;
>  
> +	if (drm_core_check_feature(drm, DRIVER_ATOMIC))
> +		connector_funcs = &tda998x_connector_atomic_funcs;
> +	else
> +		connector_funcs = &tda998x_connector_funcs;
> +
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> @@ -1437,7 +1450,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
>  	drm_connector_helper_add(&priv->connector,
>  				 &tda998x_connector_helper_funcs);
>  	ret = drm_connector_init(drm, &priv->connector,
> -				 &tda998x_connector_funcs,
> +				 connector_funcs,
>  				 DRM_MODE_CONNECTOR_HDMIA);
>  	if (ret)
>  		goto err_connector;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Jyri Sarha Jan. 16, 2016, 7:55 p.m. UTC | #2
On 01/15/16 16:09, Liviu.Dudau@arm.com wrote:
> On Fri, Jan 15, 2016 at 02:55:26PM +0200, Jyri Sarha wrote:
>> I am not sure if this is the best way to fix the issue, but after
>> 9736e988d328 Beaglebone-Black HDMI is totally broken and this fixes
>> the issue. I am currently working on atomic modesetting for tilcdc,
>> but there is no way it makes it to 4.5 release.
>
> Yes, I'm not a big fan of the change either. I would've thought that
> you only need to update the .dpms pointer, all other hooks added are
> specific to atomic operations (right?). Would it not be simpler to have
> a tda998x_connector_dpms() function that calls the appropriate .dpms
> function based on the feature check?
>

Oh yes, that sound like a better idea, but si and appears to work too. 
I'll send another patch soon. Thanks.

Best regards,
Jyri

> Best regards,
> Liviu
>
>>
>> Best regards,
>> Jyri
>>
>>   drivers/gpu/drm/i2c/tda998x_drv.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index 012d36d..67d1408 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -1382,7 +1382,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
>>   	drm_connector_cleanup(connector);
>>   }
>>
>> -static const struct drm_connector_funcs tda998x_connector_funcs = {
>> +static const struct drm_connector_funcs tda998x_connector_atomic_funcs = {
>>   	.dpms = drm_atomic_helper_connector_dpms,
>>   	.reset = drm_atomic_helper_connector_reset,
>>   	.fill_modes = drm_helper_probe_single_connector_modes,
>> @@ -1392,15 +1392,28 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
>>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>   };
>>
>> +static const struct drm_connector_funcs tda998x_connector_funcs = {
>> +	.dpms = drm_helper_connector_dpms,
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.detect = tda998x_connector_detect,
>> +	.destroy = tda998x_connector_destroy,make[4]: *** [drivers/g
>> +};
>> +
>>   static int tda998x_bind(struct device *dev, struct device *master, void *data)
>>   {
>>   	struct tda998x_encoder_params *params = dev->platform_data;
>>   	struct i2c_client *client = to_i2c_client(dev);
>>   	struct drm_device *drm = data;
>>   	struct tda998x_priv *priv;
>> +	const struct drm_connector_funcs *connector_funcs;
>>   	u32 crtcs = 0;
>>   	int ret;
>>   make[4]: *** [drivers/g
>> +	if (drm_core_check_feature(drm, DRIVER_ATOMIC))
>> +		connector_funcs = &tda998x_connector_atomic_funcs;
>> +	else
>> +		connector_funcs = &tda998x_connector_funcs;
>> +
>>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>   	if (!priv)
>>   		return -ENOMEM;
>> @@ -1437,7 +1450,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
>>   	drm_connector_helper_add(&priv->connector,
>>   				 &tda998x_connector_helper_funcs);
>>   	ret = drm_connector_init(drm, &priv->connector,
>> -				 &tda998x_connector_funcs,
>> +				 connector_funcs,
>>   				 DRM_MODE_CONNECTOR_HDMIA);
>>   	if (ret)
>>   		goto err_connector;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 012d36d..67d1408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1382,7 +1382,7 @@  static void tda998x_connector_destroy(struct drm_connector *connector)
 	drm_connector_cleanup(connector);
 }
 
-static const struct drm_connector_funcs tda998x_connector_funcs = {
+static const struct drm_connector_funcs tda998x_connector_atomic_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1392,15 +1392,28 @@  static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static const struct drm_connector_funcs tda998x_connector_funcs = {
+	.dpms = drm_helper_connector_dpms,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = tda998x_connector_detect,
+	.destroy = tda998x_connector_destroy,
+};
+
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
 	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct drm_device *drm = data;
 	struct tda998x_priv *priv;
+	const struct drm_connector_funcs *connector_funcs;
 	u32 crtcs = 0;
 	int ret;
 
+	if (drm_core_check_feature(drm, DRIVER_ATOMIC))
+		connector_funcs = &tda998x_connector_atomic_funcs;
+	else
+		connector_funcs = &tda998x_connector_funcs;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -1437,7 +1450,7 @@  static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	drm_connector_helper_add(&priv->connector,
 				 &tda998x_connector_helper_funcs);
 	ret = drm_connector_init(drm, &priv->connector,
-				 &tda998x_connector_funcs,
+				 connector_funcs,
 				 DRM_MODE_CONNECTOR_HDMIA);
 	if (ret)
 		goto err_connector;