[tiL4.19-AD PATCH 0/2] drm/tilcdc: a cleanup and a fix

[tiL4.19-AD PATCH 0/2] drm/tilcdc: a cleanup and a fix

[Jyri Sarha]

Jyri Sarha (2):
  drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value
  drm/tilcdc: Remove obsolete crtc_mode_valid() hack

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c     | 30 ++++----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.h      |  2 -
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 88 +++---------------------
 drivers/gpu/drm/tilcdc/tilcdc_external.h |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c    |  9 ---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c   |  9 ---
 7 files changed, 21 insertions(+), 119 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[tiL4.19-AD PATCH 1/2] drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value

[Jyri Sarha]

drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
checked before dereferencing the returned pointer.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 1067e702c22c..a8ae064f52bb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	u64 dma_base_and_ceiling;
 
 	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (WARN_ON(!gem))
+		return;
 
 	start = gem->paddr + fb->offsets[0] +
 		crtc->y * fb->pitches[0] +
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [tiL4.19-AD PATCH 1/2] drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
> checked before dereferencing the returned pointer.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 1067e702c22c..a8ae064f52bb 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  	u64 dma_base_and_ceiling;
>  
>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +	if (WARN_ON(!gem))
> +		return;

But this should not happen, right ? Don't we have the required checks in
place to ensure there will always be a valid GEM object available here ?

>  	start = gem->paddr + fb->offsets[0] +
>  		crtc->y * fb->pitches[0] +

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [tiL4.19-AD PATCH 1/2] drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value

[Jyri Sarha]

On 05/03/2019 17:43, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
>> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
>> checked before dereferencing the returned pointer.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 1067e702c22c..a8ae064f52bb 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>>  	u64 dma_base_and_ceiling;
>>  
>>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
>> +	if (WARN_ON(!gem))
>> +		return;
> 
> But this should not happen, right ? Don't we have the required checks in
> place to ensure there will always be a valid GEM object available here ?
> 

The patch is trying to make clockwork - a static analysis tool - happy.
Should have mentioned that in the patch. But the fact that static
clockwork complains about this suggests that either there is no such
check elsewhere, or clockwork does not understand it.

>>  	start = gem->paddr + fb->offsets[0] +
>>  		crtc->y * fb->pitches[0] +
> 

Best regards,
Jyri



-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [tiL4.19-AD PATCH 1/2] drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value

[Laurent Pinchart]

On Tue, Mar 05, 2019 at 09:42:50PM +0200, Jyri Sarha wrote:
> On 05/03/2019 17:43, Laurent Pinchart wrote:
> > Hi Jyri,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
> >> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
> >> checked before dereferencing the returned pointer.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> index 1067e702c22c..a8ae064f52bb 100644
> >> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
> >>  	u64 dma_base_and_ceiling;
> >>  
> >>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
> >> +	if (WARN_ON(!gem))
> >> +		return;
> > 
> > But this should not happen, right ? Don't we have the required checks in
> > place to ensure there will always be a valid GEM object available here ?
> > 
> 
> The patch is trying to make clockwork - a static analysis tool - happy.
> Should have mentioned that in the patch. But the fact that static
> clockwork complains about this suggests that either there is no such
> check elsewhere, or clockwork does not understand it.

I'd go for the latter. drm_fb_cma_get_gem_obj() will return NULL if the
plane index is >= 4 (which is clearly not the case here), and will
otherwise return the GEM object pointer from an internal array. That
array is populated when the framebuffer is allocated created, by
drm_gem_fb_create() called from tilcdc_fb_create() in this case. The
check here is not needed, but clockwork can't easily know about that.

> >>  	start = gem->paddr + fb->offsets[0] +
> >>  		crtc->y * fb->pitches[0] +

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[tiL4.19-AD PATCH 2/2] drm/tilcdc: Remove obsolete crtc_mode_valid() hack

[Jyri Sarha]

Earlier there were no mode_valid() helper for crtc and tilcdc had a
hack to over come this limitation. But now the mode_valid() helper is
there (has been since v4.13), so it is about time to get rid of that
hack.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c     | 28 +++-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.h      |  2 -
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 88 +++---------------------
 drivers/gpu/drm/tilcdc/tilcdc_external.h |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_panel.c    |  9 ---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c   |  9 ---
 7 files changed, 19 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index a8ae064f52bb..f9600f2ad660 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -659,9 +659,6 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
 				    struct drm_crtc_state *state)
 {
-	struct drm_display_mode *mode = &state->mode;
-	int ret;
-
 	/* If we are not active we don't care */
 	if (!state->active)
 		return 0;
@@ -673,12 +670,6 @@ static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	ret = tilcdc_crtc_mode_valid(crtc, mode);
-	if (ret) {
-		dev_dbg(crtc->dev->dev, "Mode \"%s\" not valid", mode->name);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -730,13 +721,6 @@ static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
 	.disable_vblank	= tilcdc_crtc_disable_vblank,
 };
 
-static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
-		.mode_fixup     = tilcdc_crtc_mode_fixup,
-		.atomic_check	= tilcdc_crtc_atomic_check,
-		.atomic_enable	= tilcdc_crtc_atomic_enable,
-		.atomic_disable	= tilcdc_crtc_atomic_disable,
-};
-
 int tilcdc_crtc_max_width(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -751,7 +735,9 @@ int tilcdc_crtc_max_width(struct drm_crtc *crtc)
 	return max_width;
 }
 
-int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode)
+static enum drm_mode_status
+tilcdc_crtc_mode_valid(struct drm_crtc *crtc,
+		       const struct drm_display_mode *mode)
 {
 	struct tilcdc_drm_private *priv = crtc->dev->dev_private;
 	unsigned int bandwidth;
@@ -839,6 +825,14 @@ int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode)
 	return MODE_OK;
 }
 
+static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
+		.mode_valid	= tilcdc_crtc_mode_valid,
+		.mode_fixup	= tilcdc_crtc_mode_fixup,
+		.atomic_check	= tilcdc_crtc_atomic_check,
+		.atomic_enable	= tilcdc_crtc_atomic_enable,
+		.atomic_disable	= tilcdc_crtc_atomic_disable,
+};
+
 void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
 		const struct tilcdc_panel_info *info)
 {
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 3dac08b24140..36fd05b145a4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -192,7 +192,6 @@ static void tilcdc_fini(struct drm_device *dev)
 	drm_kms_helper_poll_fini(dev);
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
-	tilcdc_remove_external_device(dev);
 
 #ifdef CONFIG_CPU_FREQ
 	if (priv->freq_transition.notifier_call)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 62cea5ff5558..3298f39d320e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -86,7 +86,6 @@ struct tilcdc_drm_private {
 
 	struct drm_encoder *external_encoder;
 	struct drm_connector *external_connector;
-	const struct drm_connector_helper_funcs *connector_funcs;
 
 	bool is_registered;
 	bool is_componentized;
@@ -168,7 +167,6 @@ void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
 		const struct tilcdc_panel_info *info);
 void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
 					bool simulate_vesa_sync);
-int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
 void tilcdc_crtc_shutdown(struct drm_crtc *crtc);
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index b4eaf9bc87f8..54adf05f44e6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -40,64 +40,6 @@ static const struct tilcdc_panel_info panel_info_default = {
 		.raster_order           = 0,
 };
 
-static int tilcdc_external_mode_valid(struct drm_connector *connector,
-				      struct drm_display_mode *mode)
-{
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	int ret;
-
-	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
-	if (ret != MODE_OK)
-		return ret;
-
-	BUG_ON(priv->external_connector != connector);
-	BUG_ON(!priv->connector_funcs);
-
-	/* If the connector has its own mode_valid call it. */
-	if (!IS_ERR(priv->connector_funcs) &&
-	    priv->connector_funcs->mode_valid)
-		return priv->connector_funcs->mode_valid(connector, mode);
-
-	return MODE_OK;
-}
-
-static int tilcdc_add_external_connector(struct drm_device *dev,
-					 struct drm_connector *connector)
-{
-	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct drm_connector_helper_funcs *connector_funcs;
-
-	/* There should never be more than one connector */
-	if (WARN_ON(priv->external_connector))
-		return -EINVAL;
-
-	priv->external_connector = connector;
-	connector_funcs = devm_kzalloc(dev->dev, sizeof(*connector_funcs),
-				       GFP_KERNEL);
-	if (!connector_funcs)
-		return -ENOMEM;
-
-	/* connector->helper_private contains always struct
-	 * connector_helper_funcs pointer. For tilcdc crtc to have a
-	 * say if a specific mode is Ok, we need to install our own
-	 * helper functions. In our helper functions we copy
-	 * everything else but use our own mode_valid() (above).
-	 */
-	if (connector->helper_private) {
-		priv->connector_funcs =	connector->helper_private;
-		*connector_funcs = *priv->connector_funcs;
-	} else {
-		priv->connector_funcs = ERR_PTR(-ENOENT);
-	}
-	connector_funcs->mode_valid = tilcdc_external_mode_valid;
-	drm_connector_helper_add(connector, connector_funcs);
-
-	dev_dbg(dev->dev, "External connector '%s' connected\n",
-		connector->name);
-
-	return 0;
-}
-
 static
 struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
 						    struct drm_encoder *encoder)
@@ -118,7 +60,6 @@ struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
 int tilcdc_add_component_encoder(struct drm_device *ddev)
 {
 	struct tilcdc_drm_private *priv = ddev->dev_private;
-	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 
 	list_for_each_entry(encoder, &ddev->mode_config.encoder_list, head)
@@ -130,28 +71,17 @@ int tilcdc_add_component_encoder(struct drm_device *ddev)
 		return -ENODEV;
 	}
 
-	connector = tilcdc_encoder_find_connector(ddev, encoder);
+	priv->external_connector
+		= tilcdc_encoder_find_connector(ddev, encoder);
 
-	if (!connector)
+	if (!priv->external_connector)
 		return -ENODEV;
 
 	/* Only tda998x is supported at the moment. */
 	tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
 	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_tda998x);
 
-	return tilcdc_add_external_connector(ddev, connector);
-}
-
-void tilcdc_remove_external_device(struct drm_device *dev)
-{
-	struct tilcdc_drm_private *priv = dev->dev_private;
-
-	/* Restore the original helper functions, if any. */
-	if (IS_ERR(priv->connector_funcs))
-		drm_connector_helper_add(priv->external_connector, NULL);
-	else if (priv->connector_funcs)
-		drm_connector_helper_add(priv->external_connector,
-					 priv->connector_funcs);
+	return 0;
 }
 
 static const struct drm_encoder_funcs tilcdc_external_encoder_funcs = {
@@ -162,7 +92,6 @@ static
 int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
 {
 	struct tilcdc_drm_private *priv = ddev->dev_private;
-	struct drm_connector *connector;
 	int ret;
 
 	priv->external_encoder->possible_crtcs = BIT(0);
@@ -175,13 +104,12 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
 
 	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_default);
 
-	connector = tilcdc_encoder_find_connector(ddev, priv->external_encoder);
-	if (!connector)
+	priv->external_connector =
+		tilcdc_encoder_find_connector(ddev, priv->external_encoder);
+	if (!priv->external_connector)
 		return -ENODEV;
 
-	ret = tilcdc_add_external_connector(ddev, connector);
-
-	return ret;
+	return 0;
 }
 
 int tilcdc_attach_external_device(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
index 763d18f006c7..a28b9df68c8f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
@@ -19,7 +19,6 @@
 #define __TILCDC_EXTERNAL_H__
 
 int tilcdc_add_component_encoder(struct drm_device *dev);
-void tilcdc_remove_external_device(struct drm_device *dev);
 int tilcdc_get_external_components(struct device *dev,
 				   struct component_match **match);
 int tilcdc_attach_external_device(struct drm_device *ddev);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index a1acab39d87f..7c26f81fb3f0 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -170,14 +170,6 @@ static int panel_connector_get_modes(struct drm_connector *connector)
 	return i;
 }
 
-static int panel_connector_mode_valid(struct drm_connector *connector,
-		  struct drm_display_mode *mode)
-{
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	/* our only constraints are what the crtc can generate: */
-	return tilcdc_crtc_mode_valid(priv->crtc, mode);
-}
-
 static struct drm_encoder *panel_connector_best_encoder(
 		struct drm_connector *connector)
 {
@@ -195,7 +187,6 @@ static const struct drm_connector_funcs panel_connector_funcs = {
 
 static const struct drm_connector_helper_funcs panel_connector_helper_funcs = {
 	.get_modes          = panel_connector_get_modes,
-	.mode_valid         = panel_connector_mode_valid,
 	.best_encoder       = panel_connector_best_encoder,
 };
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index daebf1aa6b0a..54c6d825b1a0 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -183,14 +183,6 @@ static int tfp410_connector_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
-static int tfp410_connector_mode_valid(struct drm_connector *connector,
-		  struct drm_display_mode *mode)
-{
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	/* our only constraints are what the crtc can generate: */
-	return tilcdc_crtc_mode_valid(priv->crtc, mode);
-}
-
 static struct drm_encoder *tfp410_connector_best_encoder(
 		struct drm_connector *connector)
 {
@@ -209,7 +201,6 @@ static const struct drm_connector_funcs tfp410_connector_funcs = {
 
 static const struct drm_connector_helper_funcs tfp410_connector_helper_funcs = {
 	.get_modes          = tfp410_connector_get_modes,
-	.mode_valid         = tfp410_connector_mode_valid,
 	.best_encoder       = tfp410_connector_best_encoder,
 };
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [tiL4.19-AD PATCH 2/2] drm/tilcdc: Remove obsolete crtc_mode_valid() hack

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Thu, Feb 28, 2019 at 01:18:51PM +0200, Jyri Sarha wrote:
> Earlier there were no mode_valid() helper for crtc and tilcdc had a
> hack to over come this limitation. But now the mode_valid() helper is
> there (has been since v4.13), so it is about time to get rid of that
> hack.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c     | 28 +++-----
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c      |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h      |  2 -
>  drivers/gpu/drm/tilcdc/tilcdc_external.c | 88 +++---------------------
>  drivers/gpu/drm/tilcdc/tilcdc_external.h |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c    |  9 ---
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c   |  9 ---
>  7 files changed, 19 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index a8ae064f52bb..f9600f2ad660 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -659,9 +659,6 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
>  static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *state)
>  {
> -	struct drm_display_mode *mode = &state->mode;
> -	int ret;
> -
>  	/* If we are not active we don't care */
>  	if (!state->active)
>  		return 0;
> @@ -673,12 +670,6 @@ static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	ret = tilcdc_crtc_mode_valid(crtc, mode);
> -	if (ret) {
> -		dev_dbg(crtc->dev->dev, "Mode \"%s\" not valid", mode->name);
> -		return -EINVAL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -730,13 +721,6 @@ static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
>  	.disable_vblank	= tilcdc_crtc_disable_vblank,
>  };
>  
> -static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
> -		.mode_fixup     = tilcdc_crtc_mode_fixup,
> -		.atomic_check	= tilcdc_crtc_atomic_check,
> -		.atomic_enable	= tilcdc_crtc_atomic_enable,
> -		.atomic_disable	= tilcdc_crtc_atomic_disable,
> -};
> -
>  int tilcdc_crtc_max_width(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -751,7 +735,9 @@ int tilcdc_crtc_max_width(struct drm_crtc *crtc)
>  	return max_width;
>  }
>  
> -int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode)
> +static enum drm_mode_status
> +tilcdc_crtc_mode_valid(struct drm_crtc *crtc,
> +		       const struct drm_display_mode *mode)
>  {
>  	struct tilcdc_drm_private *priv = crtc->dev->dev_private;
>  	unsigned int bandwidth;
> @@ -839,6 +825,14 @@ int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode)
>  	return MODE_OK;
>  }
>  
> +static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
> +		.mode_valid	= tilcdc_crtc_mode_valid,
> +		.mode_fixup	= tilcdc_crtc_mode_fixup,
> +		.atomic_check	= tilcdc_crtc_atomic_check,
> +		.atomic_enable	= tilcdc_crtc_atomic_enable,
> +		.atomic_disable	= tilcdc_crtc_atomic_disable,

While at it, could you fix the indentation ?

This patch looks good to me overall, nice cleanup.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Is it paving the way to removal of the custom tftp410 module ? :-)

> +};
> +
>  void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
>  		const struct tilcdc_panel_info *info)
>  {
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 3dac08b24140..36fd05b145a4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -192,7 +192,6 @@ static void tilcdc_fini(struct drm_device *dev)
>  	drm_kms_helper_poll_fini(dev);
>  	drm_irq_uninstall(dev);
>  	drm_mode_config_cleanup(dev);
> -	tilcdc_remove_external_device(dev);
>  
>  #ifdef CONFIG_CPU_FREQ
>  	if (priv->freq_transition.notifier_call)
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 62cea5ff5558..3298f39d320e 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -86,7 +86,6 @@ struct tilcdc_drm_private {
>  
>  	struct drm_encoder *external_encoder;
>  	struct drm_connector *external_connector;
> -	const struct drm_connector_helper_funcs *connector_funcs;
>  
>  	bool is_registered;
>  	bool is_componentized;
> @@ -168,7 +167,6 @@ void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
>  		const struct tilcdc_panel_info *info);
>  void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
>  					bool simulate_vesa_sync);
> -int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
>  int tilcdc_crtc_max_width(struct drm_crtc *crtc);
>  void tilcdc_crtc_shutdown(struct drm_crtc *crtc);
>  int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> index b4eaf9bc87f8..54adf05f44e6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -40,64 +40,6 @@ static const struct tilcdc_panel_info panel_info_default = {
>  		.raster_order           = 0,
>  };
>  
> -static int tilcdc_external_mode_valid(struct drm_connector *connector,
> -				      struct drm_display_mode *mode)
> -{
> -	struct tilcdc_drm_private *priv = connector->dev->dev_private;
> -	int ret;
> -
> -	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
> -	if (ret != MODE_OK)
> -		return ret;
> -
> -	BUG_ON(priv->external_connector != connector);
> -	BUG_ON(!priv->connector_funcs);
> -
> -	/* If the connector has its own mode_valid call it. */
> -	if (!IS_ERR(priv->connector_funcs) &&
> -	    priv->connector_funcs->mode_valid)
> -		return priv->connector_funcs->mode_valid(connector, mode);
> -
> -	return MODE_OK;
> -}
> -
> -static int tilcdc_add_external_connector(struct drm_device *dev,
> -					 struct drm_connector *connector)
> -{
> -	struct tilcdc_drm_private *priv = dev->dev_private;
> -	struct drm_connector_helper_funcs *connector_funcs;
> -
> -	/* There should never be more than one connector */
> -	if (WARN_ON(priv->external_connector))
> -		return -EINVAL;
> -
> -	priv->external_connector = connector;
> -	connector_funcs = devm_kzalloc(dev->dev, sizeof(*connector_funcs),
> -				       GFP_KERNEL);
> -	if (!connector_funcs)
> -		return -ENOMEM;
> -
> -	/* connector->helper_private contains always struct
> -	 * connector_helper_funcs pointer. For tilcdc crtc to have a
> -	 * say if a specific mode is Ok, we need to install our own
> -	 * helper functions. In our helper functions we copy
> -	 * everything else but use our own mode_valid() (above).
> -	 */
> -	if (connector->helper_private) {
> -		priv->connector_funcs =	connector->helper_private;
> -		*connector_funcs = *priv->connector_funcs;
> -	} else {
> -		priv->connector_funcs = ERR_PTR(-ENOENT);
> -	}
> -	connector_funcs->mode_valid = tilcdc_external_mode_valid;
> -	drm_connector_helper_add(connector, connector_funcs);
> -
> -	dev_dbg(dev->dev, "External connector '%s' connected\n",
> -		connector->name);
> -
> -	return 0;
> -}
> -
>  static
>  struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
>  						    struct drm_encoder *encoder)
> @@ -118,7 +60,6 @@ struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
>  int tilcdc_add_component_encoder(struct drm_device *ddev)
>  {
>  	struct tilcdc_drm_private *priv = ddev->dev_private;
> -	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
>  
>  	list_for_each_entry(encoder, &ddev->mode_config.encoder_list, head)
> @@ -130,28 +71,17 @@ int tilcdc_add_component_encoder(struct drm_device *ddev)
>  		return -ENODEV;
>  	}
>  
> -	connector = tilcdc_encoder_find_connector(ddev, encoder);
> +	priv->external_connector
> +		= tilcdc_encoder_find_connector(ddev, encoder);
>  
> -	if (!connector)
> +	if (!priv->external_connector)
>  		return -ENODEV;
>  
>  	/* Only tda998x is supported at the moment. */
>  	tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
>  	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_tda998x);
>  
> -	return tilcdc_add_external_connector(ddev, connector);
> -}
> -
> -void tilcdc_remove_external_device(struct drm_device *dev)
> -{
> -	struct tilcdc_drm_private *priv = dev->dev_private;
> -
> -	/* Restore the original helper functions, if any. */
> -	if (IS_ERR(priv->connector_funcs))
> -		drm_connector_helper_add(priv->external_connector, NULL);
> -	else if (priv->connector_funcs)
> -		drm_connector_helper_add(priv->external_connector,
> -					 priv->connector_funcs);
> +	return 0;
>  }
>  
>  static const struct drm_encoder_funcs tilcdc_external_encoder_funcs = {
> @@ -162,7 +92,6 @@ static
>  int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
>  {
>  	struct tilcdc_drm_private *priv = ddev->dev_private;
> -	struct drm_connector *connector;
>  	int ret;
>  
>  	priv->external_encoder->possible_crtcs = BIT(0);
> @@ -175,13 +104,12 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
>  
>  	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_default);
>  
> -	connector = tilcdc_encoder_find_connector(ddev, priv->external_encoder);
> -	if (!connector)
> +	priv->external_connector =
> +		tilcdc_encoder_find_connector(ddev, priv->external_encoder);
> +	if (!priv->external_connector)
>  		return -ENODEV;
>  
> -	ret = tilcdc_add_external_connector(ddev, connector);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  int tilcdc_attach_external_device(struct drm_device *ddev)
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
> index 763d18f006c7..a28b9df68c8f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
> @@ -19,7 +19,6 @@
>  #define __TILCDC_EXTERNAL_H__
>  
>  int tilcdc_add_component_encoder(struct drm_device *dev);
> -void tilcdc_remove_external_device(struct drm_device *dev);
>  int tilcdc_get_external_components(struct device *dev,
>  				   struct component_match **match);
>  int tilcdc_attach_external_device(struct drm_device *ddev);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> index a1acab39d87f..7c26f81fb3f0 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> @@ -170,14 +170,6 @@ static int panel_connector_get_modes(struct drm_connector *connector)
>  	return i;
>  }
>  
> -static int panel_connector_mode_valid(struct drm_connector *connector,
> -		  struct drm_display_mode *mode)
> -{
> -	struct tilcdc_drm_private *priv = connector->dev->dev_private;
> -	/* our only constraints are what the crtc can generate: */
> -	return tilcdc_crtc_mode_valid(priv->crtc, mode);
> -}
> -
>  static struct drm_encoder *panel_connector_best_encoder(
>  		struct drm_connector *connector)
>  {
> @@ -195,7 +187,6 @@ static const struct drm_connector_funcs panel_connector_funcs = {
>  
>  static const struct drm_connector_helper_funcs panel_connector_helper_funcs = {
>  	.get_modes          = panel_connector_get_modes,
> -	.mode_valid         = panel_connector_mode_valid,
>  	.best_encoder       = panel_connector_best_encoder,
>  };
>  
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> index daebf1aa6b0a..54c6d825b1a0 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -183,14 +183,6 @@ static int tfp410_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>  
> -static int tfp410_connector_mode_valid(struct drm_connector *connector,
> -		  struct drm_display_mode *mode)
> -{
> -	struct tilcdc_drm_private *priv = connector->dev->dev_private;
> -	/* our only constraints are what the crtc can generate: */
> -	return tilcdc_crtc_mode_valid(priv->crtc, mode);
> -}
> -
>  static struct drm_encoder *tfp410_connector_best_encoder(
>  		struct drm_connector *connector)
>  {
> @@ -209,7 +201,6 @@ static const struct drm_connector_funcs tfp410_connector_funcs = {
>  
>  static const struct drm_connector_helper_funcs tfp410_connector_helper_funcs = {
>  	.get_modes          = tfp410_connector_get_modes,
> -	.mode_valid         = tfp410_connector_mode_valid,
>  	.best_encoder       = tfp410_connector_best_encoder,
>  };
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [tiL4.19-AD PATCH 2/2] drm/tilcdc: Remove obsolete crtc_mode_valid() hack

[Jyri Sarha]

On 05/03/2019 21:21, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Thu, Feb 28, 2019 at 01:18:51PM +0200, Jyri Sarha wrote:
>> Earlier there were no mode_valid() helper for crtc and tilcdc had a
>> hack to over come this limitation. But now the mode_valid() helper is
>> there (has been since v4.13), so it is about time to get rid of that
>> hack.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
...
>> +static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
>> +		.mode_valid	= tilcdc_crtc_mode_valid,
>> +		.mode_fixup	= tilcdc_crtc_mode_fixup,
>> +		.atomic_check	= tilcdc_crtc_atomic_check,
>> +		.atomic_enable	= tilcdc_crtc_atomic_enable,
>> +		.atomic_disable	= tilcdc_crtc_atomic_disable,
> 
> While at it, could you fix the indentation ?
> 
> This patch looks good to me overall, nice cleanup.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Is it paving the way to removal of the custom tftp410 module ? :-)
> 

Not really. There is really no simple way to get rid of the if we want
to keep the DTB backward compatibility. There has not been any other
reason to keep it for some time. Tilcdc supports brides and there is a
bridge driver for tfp410.

BR,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel