| Message ID | 1415283007-10096-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 >
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 > >
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 >
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 :)
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
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(+)