[PATCH] drm: Change link order to load modules first

[PATCH] drm: Change link order to load modules first

[Ezequiel Garcia]

Given panels and I2C-connected encoders are required by DRM drivers,
we need to change the link order so these are probed first. This commit
moves all the i2c, panel and bridge helper drivers so they are probed
before the DRM drivers.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/gpu/drm/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index dd2ba42..552eaad 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,10 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
 CFLAGS_drm_trace_points.o := -I$(src)
 
+obj-y			+= i2c/
+obj-y			+= panel/
+obj-y			+= bridge/
+
 obj-$(CONFIG_DRM)	+= drm.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
 obj-$(CONFIG_DRM_USB)   += drm_usb.o
@@ -63,6 +67,3 @@ obj-$(CONFIG_DRM_QXL) += qxl/
 obj-$(CONFIG_DRM_BOCHS) += bochs/
 obj-$(CONFIG_DRM_MSM) += msm/
 obj-$(CONFIG_DRM_TEGRA) += tegra/
-obj-y			+= i2c/
-obj-y			+= panel/
-obj-y			+= bridge/
-- 
1.9.1

Re: [PATCH] drm: Change link order to load modules first

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 572 bytes --]

On Sun, Jun 22, 2014 at 10:14:36PM -0300, Ezequiel Garcia wrote:
> Given panels and I2C-connected encoders are required by DRM drivers,
> we need to change the link order so these are probed first. This commit
> moves all the i2c, panel and bridge helper drivers so they are probed
> before the DRM drivers.

No. We don't need to change the link order. What we need to do is make
sure that modules deal properly with situations where their resources
aren't available yet (i.e. EPROBE_DEFER). There are factors other than
link order that influence probe ordering.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm: Change link order to load modules first

[Ezequiel Garcia]

Hi Thierry,

Thanks for looking at this.

On 23 Jun 04:58 PM, Thierry Reding wrote:
> On Sun, Jun 22, 2014 at 10:14:36PM -0300, Ezequiel Garcia wrote:
> > Given panels and I2C-connected encoders are required by DRM drivers,
> > we need to change the link order so these are probed first. This commit
> > moves all the i2c, panel and bridge helper drivers so they are probed
> > before the DRM drivers.
> 
> No. We don't need to change the link order.

Could you clarify why? I guess you have some case in mind where changing
the link order breaks things or makes something mis-behave.

> What we need to do is make
> sure that modules deal properly with situations where their resources
> aren't available yet (i.e. EPROBE_DEFER). There are factors other than
> link order that influence probe ordering.
> 

While I understand defering is more robust, it would be systematically
defering the probe when the DRM driver needs an I2C encoder.

Just to name one example, the tilcdc, armada and others requiring TDA998x
encoder will always defered the probe of the DRM, and then re-probe() once
the encoder is ready.

So, unless we have a good reason not to do this, it sounds a bit silly to me.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] drm: Change link order to load modules first

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2135 bytes --]

On Mon, Jun 23, 2014 at 12:29:09PM -0300, Ezequiel Garcia wrote:
> Hi Thierry,
> 
> Thanks for looking at this.
> 
> On 23 Jun 04:58 PM, Thierry Reding wrote:
> > On Sun, Jun 22, 2014 at 10:14:36PM -0300, Ezequiel Garcia wrote:
> > > Given panels and I2C-connected encoders are required by DRM drivers,
> > > we need to change the link order so these are probed first. This commit
> > > moves all the i2c, panel and bridge helper drivers so they are probed
> > > before the DRM drivers.
> > 
> > No. We don't need to change the link order.
> 
> Could you clarify why? I guess you have some case in mind where changing
> the link order breaks things or makes something mis-behave.

I said we don't need to change the link order because there is a better
mechanism in the kernel to handle this type of situation. Saying "we
need to change" makes it sound like there's a bug that needs to be fixed
by changing the link order. That's not so. If link order breaks some
drivers then its those drivers that are broken.

> > What we need to do is make
> > sure that modules deal properly with situations where their resources
> > aren't available yet (i.e. EPROBE_DEFER). There are factors other than
> > link order that influence probe ordering.
> > 
> 
> While I understand defering is more robust, it would be systematically
> defering the probe when the DRM driver needs an I2C encoder.
> 
> Just to name one example, the tilcdc, armada and others requiring TDA998x
> encoder will always defered the probe of the DRM, and then re-probe() once
> the encoder is ready.
> 
> So, unless we have a good reason not to do this, it sounds a bit silly
> to me.

The problem that I have with working around this issue by changing the
link order is that it hides bugs in drivers. It's not like probe
deferral is a very expensive operation, so I very much prefer this as a
way of forcing drivers to be fixed rather than optimizing for a few
microseconds of boot time.

Also note that even if TDA998x is probed first that doesn't mean the
probe will succeed. It could equally well be deferred.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm: Change link order to load modules first

[Ezequiel Garcia]

On 27 Jun 07:14 AM, Thierry Reding wrote:
> On Mon, Jun 23, 2014 at 12:29:09PM -0300, Ezequiel Garcia wrote:
> > > 
> > > No. We don't need to change the link order.
> > 
> > Could you clarify why? I guess you have some case in mind where changing
> > the link order breaks things or makes something mis-behave.
> 
> I said we don't need to change the link order because there is a better
> mechanism in the kernel to handle this type of situation. Saying "we
> need to change" makes it sound like there's a bug that needs to be fixed
> by changing the link order. That's not so. If link order breaks some
> drivers then its those drivers that are broken.
> 

Ah, OK. I understand your point now. On a second read, my commit log isn't
too clear explaining why I want to change the order.

> > > What we need to do is make
> > > sure that modules deal properly with situations where their resources
> > > aren't available yet (i.e. EPROBE_DEFER). There are factors other than
> > > link order that influence probe ordering.
> > > 
> > 
> > While I understand defering is more robust, it would be systematically
> > defering the probe when the DRM driver needs an I2C encoder.
> > 
> > Just to name one example, the tilcdc, armada and others requiring TDA998x
> > encoder will always defered the probe of the DRM, and then re-probe() once
> > the encoder is ready.
> > 
> > So, unless we have a good reason not to do this, it sounds a bit silly
> > to me.
> 
> The problem that I have with working around this issue by changing the
> link order is that it hides bugs in drivers. It's not like probe
> deferral is a very expensive operation, so I very much prefer this as a
> way of forcing drivers to be fixed rather than optimizing for a few
> microseconds of boot time.
> 

That's true. However, in this particular case, I'm not worried about the
extra microseconds wasted in the defered operation, but about the unneeded
delay introduced in the DRM probe.

I know that relying in some particular order is very fragile, but I don't
agree with knowingly and even explicitly have a sub-optimal order of the
drivers.

Or maybe it's just that I dislike deferals a lot.

Anyway, thanks for feedback.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar