On 18/09/13 16:17, Archit Taneja wrote:
> On Wednesday 18 September 2013 06:11 PM, Tomi Valkeinen wrote:
>> On 18/09/13 14:08, Archit Taneja wrote:
>>> Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI)
>>> that require
>>> regulators. The output's connect op tries to get a regulator which
>>> may not exist
>>> yet because it might get registered later in the kernel boot.
>>>
>>> omapdrm currently ignores those panels which return a non zero value
>>> when
>>> connected. A better approach would be for omapdrm to request for probe
>>> deferral if a panel's connect op returns -EPROBE_DEFER.
>>>
>>> The connecting of panels is moved very early in the the drm device's
>>> probe
>>> before anything else is initialized. When we enter
>>> omap_modeset_init(), we have
>>> a set of panels that have been connected. We now proceed with
>>> registering only
>>> those panels which are already connected.
>>>
>>> Checking whether the panel has a driver or whether it has
>>> get_timing/read_edid
>>> ops in omap_modeset_init() are redundant with the new display model.
>>> These can
>>> be removed since a dssdev device will always have a driver associated
>>> with it,
>>> and all dssdev drivers have a get_timings op.
>>>
>>> This fixes boot with omapdrm on an omap4 panda ES board. The
>>> regulators used by
>>> HDMI aren't initialized because I2c isn't initialized, I2C isn't
>>> initialized
>>> as it's pins are not configured because pinctrl is yet to probe.
>>>
>>> Signed-off-by: Archit Taneja <archit@xxxxxx>
>>> ---
>>> drivers/gpu/drm/omapdrm/omap_crtc.c | 5 ++++
>>> drivers/gpu/drm/omapdrm/omap_drv.c | 51
>>> +++++++++++++++++++++----------------
>>> drivers/gpu/drm/omapdrm/omap_drv.h | 1 +
>>> 3 files changed, 35 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> index 0fd2eb1..9c01311 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> @@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
>>> dss_install_mgr_ops(&mgr_ops);
>>> }
>>>
>>> +void omap_crtc_pre_uninit(void)
>>> +{
>>> + dss_uninstall_mgr_ops();
>>> +}
>>> +
>>> /* initialize crtc */
>>> struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>>> struct drm_plane *plane, enum omap_channel channel, int id)
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> index 2603d90..cbe5d8e 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> @@ -87,6 +87,24 @@ static bool channel_used(struct drm_device *dev,
>>> enum omap_channel channel)
>>> return false;
>>> }
>>>
>>> +static int omap_connect_dssdevs(void)
>>> +{
>>> + int r;
>>> + struct omap_dss_device *dssdev = NULL;
>>> +
>>> + for_each_dss_dev(dssdev) {
>>> + r = dssdev->driver->connect(dssdev);
>>> + if (r == -EPROBE_DEFER) {
>>> + return r;
>>> + } else if (r) {
>>> + dev_warn(dssdev->dev, "could not connect display: %s\n",
>>> + dssdev->name);
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> There are two issues with this one:
>>
>> for_each_dss_dev() uses omap_dss_get_next_device(), and that will
>> increase the ref count of the current dssdev. If you interrupt the loop,
>> the ref count won't be decreased. I have a hunch that we could have
>> similar bugs elsewhere too...
>
> You are saying that the ref counts will be correct only if
> for_each_dss_dev() is fully completed?
Right.
> Maybe we can save the first dssdev which doesn't connect, save that
> dssdev and let the loop continue for the refcount to get decremented again.
>
> Or maybe use omap_dss_get_next_device in a custom loop, which takes care
> of doing a put() for the device before returning.
Well, you can just use omap_dss_put_device(dssdev) before the return.
That should fix it.
Check drivers/video/omap2/dss/display.c:omap_dss_get_next_device().
>> Second, in case of error, the dssdevs that were already connected should
>> be disconnected. Although maybe that's not important, as they'll end up
>> being connected again when the omapdrm is probed the next time.
>
> The one's that were already connected won't connect again and return an
> error which isn't EPROBE_DEFER, so that should be okay right?
Right, in practice it doesn't matter. The only issue here is that it's
not very nice if you don't clean up after getting an error =). And,
well, with modules there could be issues in some particular cases, where
leaving things connected would prevent unloading of modules.
omapdrm doesn't seem to disconnect even when it's removed normally. I
guess it'd be nicer to have similar disconnect loop as in omapfb, i.e.
just go over all the displays and disconnect them all.
Tomi
Attachment:
signature.asc
Description: OpenPGP digital signature
