| Message ID | 1452862526-32176-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 --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;
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(-)