[PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Inki Dae]

This patch resovles the infinite loop issue incurred
when Exyno drm driver is enabled but all kms drivers
are disabled on Exynos board by returning -EPROBE_DEFER
only in case that there is kms device registered.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index ecc86aa..14c6af7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -488,6 +488,12 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
 
 	mutex_lock(&drm_component_lock);
 
+	/* Do not retry to probe if there is no any kms driver regitered. */
+	if (list_empty(&drm_component_list)) {
+		mutex_unlock(&drm_component_lock);
+		return ERR_PTR(-ENODEV);
+	}
+
 	list_for_each_entry(cdev, &drm_component_list, list) {
 		/*
 		 * Add components to master only in case that crtc and
-- 
1.7.9.5

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

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Emil Velikov]

Hi Inki,

With all respect,

On 06/11/14 14:10, Inki Dae wrote:> This patch resovles the infinite
loop issue incurred
> when Exyno drm driver is enabled but all kms drivers
> are disabled on Exynos board by returning -EPROBE_DEFER
> only in case that there is kms device registered.
> 
I believe it's preferred to add changelog, the original reporter (so
that they can test) and the bug report in the commit message. Something
like the following:

v2:
 - Use drm_component_list the rather than of_machine_is_compatible to
check for presence of an Exynos SoC.

Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
References: https://bugzilla.kernel.org/show_bug.cgi?id=87691

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

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Emil Velikov]

On 06/11/14 15:44, Emil Velikov wrote:
> Hi Inki,
> 
> With all respect,
> 
> On 06/11/14 14:10, Inki Dae wrote:> This patch resovles the infinite
> loop issue incurred
>> when Exyno drm driver is enabled but all kms drivers
>> are disabled on Exynos board by returning -EPROBE_DEFER
>> only in case that there is kms device registered.
>>
> I believe it's preferred to add changelog, the original reporter (so
> that they can test) and the bug report in the commit message. Something
> like the following:
> 
> v2:
>  - Use drm_component_list the rather than of_machine_is_compatible to
> check for presence of an Exynos SoC.
> 
> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=87691
> 
I'm a genius. Please disregard the above.

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

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Sjoerd Simons]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Thu, 2014-11-06 at 23:10 +0900, Inki Dae wrote:
> This patch resovles the infinite loop issue incurred
> when Exyno drm driver is enabled but all kms drivers
> are disabled on Exynos board by returning -EPROBE_DEFER
> only in case that there is kms device registered.

It would be nice if you could explain in the commit message/comment why
returning -EPROBE_DEFER causes an infinite loop and why it's the wrong
thing to do in this case? 

Even if you know this probe will never succeed in the future (so
deferring is actually pointless), deferring really shouldn't trigger
infinte loops in calling code

> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index ecc86aa..14c6af7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -488,6 +488,12 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>  
>  	mutex_lock(&drm_component_lock);
>  
> +	/* Do not retry to probe if there is no any kms driver regitered. */
> +	if (list_empty(&drm_component_list)) {
> +		mutex_unlock(&drm_component_lock);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
>  	list_for_each_entry(cdev, &drm_component_list, list) {
>  		/*
>  		 * Add components to master only in case that crtc and



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6170 bytes --]

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Andrzej Hajda]

On 11/06/2014 03:10 PM, Inki Dae wrote:
> This patch resovles the infinite loop issue incurred
> when Exyno drm driver is enabled but all kms drivers
> are disabled on Exynos board by returning -EPROBE_DEFER
> only in case that there is kms device registered.

There are many different cases it can still fail:
- there are no matching device nodes in DT,
- some devices are present in DT, some drivers are enabled,
but they do not match,
- even if there exists some pairs device_node-driver it can fail -
exynos_drm_match_add requires higher level matches - every crtc should
have matching encoder.

I think even super-node will not solve all these issues.

The real problem here is that during probe of exynos_drm ipp driver is
successfully probed and after that exynos_drm probe fails, which causes
also removing ipp, anyway successful ipp probe triggers whole re-probe
mechanism again. It creates infinite loop.

I think moving exynos_platform_device_ipp_register after
component_master_add_with_match should solve some of the issue,
but I guess it could create new ones, or maybe not, needs checking.

On the other side I wonder if we really need to have ipp device at all,
replacing it with helpers of exynos_drm should be possible, I guess.

Regards
Andrzej

> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index ecc86aa..14c6af7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -488,6 +488,12 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>  
>  	mutex_lock(&drm_component_lock);
>  
> +	/* Do not retry to probe if there is no any kms driver regitered. */
> +	if (list_empty(&drm_component_list)) {
> +		mutex_unlock(&drm_component_lock);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
>  	list_for_each_entry(cdev, &drm_component_list, list) {
>  		/*
>  		 * Add components to master only in case that crtc and
> 

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Andrzej Hajda]

On 11/06/2014 06:08 PM, Sjoerd Simons wrote:
> On Thu, 2014-11-06 at 23:10 +0900, Inki Dae wrote:
>> This patch resovles the infinite loop issue incurred
>> when Exyno drm driver is enabled but all kms drivers
>> are disabled on Exynos board by returning -EPROBE_DEFER
>> only in case that there is kms device registered.
> 
> It would be nice if you could explain in the commit message/comment why
> returning -EPROBE_DEFER causes an infinite loop and why it's the wrong
> thing to do in this case? 
> 
> Even if you know this probe will never succeed in the future (so
> deferring is actually pointless), deferring really shouldn't trigger
> infinte loops in calling code
> 

+CC: Grant and Greg

It seems to be partly an issue with deferred probing. I guess it affects
all drivers which in their probe can cause successful probe of
sub-driver/sub-device, and after that the main driver defers probing,
unwinding changes including removal of sub-driver/sub-device. Driver
core incorrectly re-triggers probing in this case.

In this particular case it could be fixed in exynos_drm driver I guess,
but maybe it would be good to fix deferred probing, if possible.

Regards
Andrzej


>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index ecc86aa..14c6af7 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -488,6 +488,12 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>>  
>>  	mutex_lock(&drm_component_lock);
>>  
>> +	/* Do not retry to probe if there is no any kms driver regitered. */
>> +	if (list_empty(&drm_component_list)) {
>> +		mutex_unlock(&drm_component_lock);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>>  	list_for_each_entry(cdev, &drm_component_list, list) {
>>  		/*
>>  		 * Add components to master only in case that crtc and
> 
> 

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Inki Dae]

On 2014년 11월 07일 17:29, Andrzej Hajda wrote:
> On 11/06/2014 03:10 PM, Inki Dae wrote:
>> This patch resovles the infinite loop issue incurred
>> when Exyno drm driver is enabled but all kms drivers
>> are disabled on Exynos board by returning -EPROBE_DEFER
>> only in case that there is kms device registered.
> 
> There are many different cases it can still fail:
> - there are no matching device nodes in DT,

With this patch, -ENODEV would be returned instead of -EPROBE_DEFER in
above case. So it should be no problem.

> - some devices are present in DT, some drivers are enabled,
> but they do not match,

Right.

> - even if there exists some pairs device_node-driver it can fail -
> exynos_drm_match_add requires higher level matches - every crtc should
> have matching encoder.

Also right.

> 
> I think even super-node will not solve all these issues.
> 

With the super device node, my intention is that we can remove
unnecessary exception codes like of_machine_is_compatible functions. So
the use of the super device node wouldn't be relevant to this issue.

> The real problem here is that during probe of exynos_drm ipp driver is
> successfully probed and after that exynos_drm probe fails, which causes
> also removing ipp, anyway successful ipp probe triggers whole re-probe
> mechanism again. It creates infinite loop.

The problem is not because of only IPP driver. In all cases that non kms
drivers are enabled and they are probed.

> 
> I think moving exynos_platform_device_ipp_register after
> component_master_add_with_match should solve some of the issue,

g2d also should be moved.

> but I guess it could create new ones, or maybe not, needs checking.
> 
> On the other side I wonder if we really need to have ipp device at all,
> replacing it with helpers of exynos_drm should be possible, I guess.
> 

Agree. I also think that we don't really need IPP driver. Instead, we
can register post processor drivers - fimc, rotator, and gscaler -
directly without IPP driver like g2d did.

Thanks,
Inki Dae

> Regards
> Andrzej
> 
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index ecc86aa..14c6af7 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -488,6 +488,12 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>>  
>>  	mutex_lock(&drm_component_lock);
>>  
>> +	/* Do not retry to probe if there is no any kms driver regitered. */
>> +	if (list_empty(&drm_component_list)) {
>> +		mutex_unlock(&drm_component_lock);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>>  	list_for_each_entry(cdev, &drm_component_list, list) {
>>  		/*
>>  		 * Add components to master only in case that crtc and
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Greg Kroah-Hartman]

On Fri, Nov 07, 2014 at 12:11:24PM +0100, Andrzej Hajda wrote:
> On 11/06/2014 06:08 PM, Sjoerd Simons wrote:
> > On Thu, 2014-11-06 at 23:10 +0900, Inki Dae wrote:
> >> This patch resovles the infinite loop issue incurred
> >> when Exyno drm driver is enabled but all kms drivers
> >> are disabled on Exynos board by returning -EPROBE_DEFER
> >> only in case that there is kms device registered.
> > 
> > It would be nice if you could explain in the commit message/comment why
> > returning -EPROBE_DEFER causes an infinite loop and why it's the wrong
> > thing to do in this case? 
> > 
> > Even if you know this probe will never succeed in the future (so
> > deferring is actually pointless), deferring really shouldn't trigger
> > infinte loops in calling code
> > 
> 
> +CC: Grant and Greg
> 
> It seems to be partly an issue with deferred probing. I guess it affects
> all drivers which in their probe can cause successful probe of
> sub-driver/sub-device, and after that the main driver defers probing,
> unwinding changes including removal of sub-driver/sub-device. Driver
> core incorrectly re-triggers probing in this case.
> 
> In this particular case it could be fixed in exynos_drm driver I guess,
> but maybe it would be good to fix deferred probing, if possible.

Patches are always gladly accepted :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel