Re: [PATCH v12 1/3] drm: rockchip: Add basic drm driver
From: Boris Brezillon
Date: Wed Nov 19 2014 - 04:04:36 EST
On Wed, 19 Nov 2014 10:02:53 +0800
Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:
> On 2014å11æ19æ 09:09, Mark yao wrote:
> > On 2014å11æ18æ 22:24, Daniel Vetter wrote:
> >> On Tue, Nov 18, 2014 at 02:21:30PM +0100, Boris Brezillon wrote:
> >>> Hi Daniel,
> >>>
> >>> On Tue, 18 Nov 2014 09:32:34 +0100
> >>> Daniel Vetter <daniel@xxxxxxxx> wrote:
> >>>
> >>>> On Tue, Nov 18, 2014 at 04:00:29PM +0800, Mark Yao wrote:
> >>>>> From: Mark yao <mark.yao@xxxxxxxxxxxxxx>
> >>>>>
> >>>>> This patch adds the basic structure of a DRM Driver for Rockchip
> >>>>> Socs.
> >>>>>
> >>>>> Signed-off-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> >>>>> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> >>>>> Acked-by: Daniel Vetter <daniel@xxxxxxxx>
> >>>>> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> - use the component framework to defer main drm driver probe
> >>>>> until all VOP devices have been probed.
> >>>>> - use dma-mapping API with ARM_DMA_USE_IOMMU, create dma mapping by
> >>>>> master device and each vop device can shared the drm dma mapping.
> >>>>> - use drm_crtc_init_with_planes and drm_universal_plane_init.
> >>>>> - remove unnecessary middle layers.
> >>>>> - add cursor set, move funcs to rockchip drm crtc.
> >>>>> - use vop reset at first init
> >>>>> - reference framebuffer when used and unreference when swap out vop
> >>>>>
> >>>>> Changes in v3:
> >>>>> - change "crtc->fb" to "crtc->primary-fb"
> >>>>> Adviced by Daniel Vetter
> >>>>> - init cursor plane with universal api, remove unnecessary cursor
> >>>>> set,move
> >>>>>
> >>>>> Changes in v4:
> >>>>> Adviced by David Herrmann
> >>>>> - remove drm_platform_*() usage, use register drm device directly.
> >>>> Minor fixup for that part below.
> >>>>
> >>>> [snip]
> >>>>
> >>>>> +static int rockchip_drm_bind(struct device *dev)
> >>>>> +{
> >>>>> + struct drm_device *drm;
> >>>>> + int ret;
> >>>>> +
> >>>>> + drm = drm_dev_alloc(&rockchip_drm_driver, dev);
> >>>>> + if (!drm)
> >>>>> + return -ENOMEM;
> >>>>> +
> >>>>> + ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
> >>>>> + if (ret)
> >>>>> + goto err_free;
> >>>> Please call rockchip_drm_load here directly and don't put it as the
> >>>> ->load
> >>>> function into the driver vtable. The point of the alloc/register
> >>>> split is
> >>>> that the driver can be completely set up _before_ we register
> >>>> anything.
> >>>> But for backwards compat and historical reasons ->load is called
> >>>> somewhere
> >>>> in the middle (so that you could access the minor nodes if needed,
> >>>> since
> >>>> some drivers do that).
> >>> I tried to do the same in the atmel-hlcdc DRM driver, but I need the
> >>> primary drm_minor to register the connectors (see this kernel
> >>> backtrace [1]), which means I either initialize the connector in the
> >>> wrong place (currently part of the drm load process), or I just can't
> >>> call atmel_hlcdc_dc_load before registering the drm device...
> >> Hm right, wonder who that works with rockchip tbh.
> >>
> >> We did split up the drm_connector setup into _init and register, so
> >> if you
> >> want to do this then you need to move the call to drm_connector_register
> >> below the call to drm_dev_register.
> >>
> >> We should probably have a drm_connectors_register_all helper which does
> >> this for all connectors on the connector list. And also grabs the
> >> appropriate lock.
> >>
> >> I guess it's somewhat obvious that no one yet actually tried this ;-)
> >> -Daniel
> > right, I re-test the driver with it, get the same problem with Boris.
> > I should move drm_connector_register below the drm_dev_register.
> I try move connector_register below drm_dev_register, but unfortunately,
> drm_dev_register call
> drm_mode_group_init_legacy_group to save crtc, connector into list, it
> need below connector_register.
> so that problem is that now: connector_register need bewteen minor node
> create and
> drm_mode_group_init_legacy_group.
AFAIU, the suggestion was to split drm_connector_init and
drm_connector_register calls:
- drm_connector_init call should still be part of the load procedure
(this function adds the connector to the connector list which is used
by drm_mode_group_init_legacy_group)
- drm_connector_register should be called after the device has been
registered
Here what I've done and it seems to work:
static int atmel_hlcdc_dc_connector_plug_all(struct drm_device *dev)
{
struct drm_connector *connector, *failed;
int ret;
mutex_lock(&dev->mode_config.mutex);
list_for_each_entry(connector,
&dev->mode_config.connector_list, head) { ret =
drm_connector_register(connector); if (ret) {
failed = connector;
goto err;
}
}
mutex_unlock(&dev->mode_config.mutex);
return 0;
err:
list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
if (failed == connector)
break;
drm_connector_unregister(connector);
}
mutex_unlock(&dev->mode_config.mutex);
return ret;
}
[...]
static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
{
struct drm_device *ddev;
int ret;
ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
if (!ddev)
return -ENOMEM;
ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
if (ret)
goto err_unref;
ret = atmel_hlcdc_dc_load(ddev);
if (ret)
goto err_unref;
ret = drm_dev_register(ddev, 0);
if (ret)
goto err_unload;
ret = atmel_hlcdc_dc_connector_plug_all(ddev);
if (ret)
goto err_unregister;
return 0;
err_unregister:
drm_dev_unregister(ddev);
err_unload:
atmel_hlcdc_dc_unload(ddev);
err_unref:
drm_dev_unref(ddev);
return ret;
}
Daniel, can you confirm that's what you had in mind ?
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/