[PATCH 12/23] drm/exynos: Split manager/display/subdrv
Inki Dae
inki.dae at samsung.com
Tue Oct 15 06:09:49 CEST 2013
2013/10/15 Inki Dae <inki.dae at samsung.com>:
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> index c417c90..ba63c72 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> @@ -26,24 +26,23 @@
>> * exynos specific encoder structure.
>> *
>> * @drm_encoder: encoder object.
>> - * @manager: specific encoder has its own manager to control a hardware
>> - * appropriately and we can access a hardware drawing on this manager.
>> + * @display: the display structure that maps to this encoder
>> */
>> struct exynos_drm_encoder {
>> struct drm_crtc *old_crtc;
>> struct drm_encoder drm_encoder;
>> - struct exynos_drm_manager *manager;
>> + struct exynos_drm_display *display;
>> };
>>
>> static void exynos_drm_encoder_dpms(struct drm_encoder *encoder, int mode)
>> {
>> - struct exynos_drm_manager *manager = exynos_drm_get_manager(encoder);
>> - struct exynos_drm_display_ops *display_ops = manager->display_ops;
>> + struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
>> + struct exynos_drm_display *display = exynos_encoder->display;
>>
>> DRM_DEBUG_KMS("encoder dpms: %d\n", mode);
>>
>> - if (display_ops && display_ops->dpms)
>> - display_ops->dpms(manager->ctx, mode);
>> + if (display->ops->dpms)
>> + display->ops->dpms(display->ctx, mode);
>
> It's good to remove apply callback. However, it seems that this patch
> has a problem that dma channel of fimd isn't enabled after dpms goes
> from off to on. So can you implement win_enable callback of fimd, and
> add it to fimd_win_resume function? We should have implemented
> win_enable callback.
>
Plz, ignore the above comments, and see the below comments.
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 838c47d..f3dc808 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -105,7 +105,6 @@ struct fimd_win_data {
>> };
>>
>> struct fimd_context {
>> - struct exynos_drm_subdrv subdrv;
>> struct device *dev;
>> struct drm_device *drm_dev;
>> int irq;
>> @@ -120,6 +119,7 @@ struct fimd_context {
>> u32 vidcon0;
>> u32 vidcon1;
>> bool suspended;
>> + int pipe;
>> struct mutex lock;
>> wait_queue_head_t wait_vsync_queue;
>> atomic_t wait_vsync_event;
>> @@ -169,12 +169,16 @@ static int fimd_check_mode(void *in_ctx, struct drm_display_mode *mode)
>> }
>>
>> static struct exynos_drm_display_ops fimd_display_ops = {
>> - .type = EXYNOS_DISPLAY_TYPE_LCD,
>> .is_connected = fimd_display_is_connected,
>> .get_panel = fimd_get_panel,
>> .check_mode = fimd_check_mode,
>> };
>>
>> +static struct exynos_drm_display fimd_display = {
>> + .type = EXYNOS_DISPLAY_TYPE_LCD,
>> + .ops = &fimd_display_ops,
>> +};
>> +
>> static void fimd_win_mode_set(void *in_ctx, struct exynos_drm_overlay *overlay)
>> {
>> struct fimd_context *ctx = in_ctx;
>> @@ -481,15 +485,46 @@ static void fimd_win_disable(void *in_ctx, int zpos)
>> win_data->enabled = false;
>> }
>>
>> -static int fimd_mgr_initialize(void *in_ctx, struct drm_device *drm_dev)
>> +static int fimd_mgr_initialize(void *in_ctx, struct drm_device *drm_dev,
>> + int pipe)
>> {
>> struct fimd_context *ctx = in_ctx;
>>
>> ctx->drm_dev = drm_dev;
>> + ctx->pipe = pipe;
>> +
>> + /*
>> + * enable drm irq mode.
>> + * - with irq_enabled = 1, we can use the vblank feature.
>> + *
>> + * P.S. note that we wouldn't use drm irq handler but
>> + * just specific driver own one instead because
>> + * drm framework supports only one irq handler.
>> + */
>> + ctx->drm_dev->irq_enabled = 1;
>> +
>> + /*
>> + * with vblank_disable_allowed = 1, vblank interrupt will be disabled
>> + * by drm timer once a current process gives up ownership of
>> + * vblank event.(after drm_vblank_put function is called)
>> + */
>> + drm_dev->vblank_disable_allowed = 1;
>> +
>> + /* attach this sub driver to iommu mapping if supported. */
>> + if (is_drm_iommu_supported(ctx->drm_dev))
>> + drm_iommu_attach_device(ctx->drm_dev, ctx->dev);
>>
>> return 0;
>> }
>>
>> +static void fimd_mgr_remove(void *in_ctx)
>> +{
>> + struct fimd_context *ctx = in_ctx;
>> +
>> + if (is_drm_iommu_supported(ctx->drm_dev))
>> + drm_iommu_detach_device(ctx->drm_dev, ctx->dev);
>> +}
>> +
>> static void fimd_dpms(void *in_ctx, int mode)
>> {
>> struct fimd_context *ctx = in_ctx;
>> @@ -523,24 +558,6 @@ static void fimd_dpms(void *in_ctx, int mode)
>> mutex_unlock(&ctx->lock);
>> }
>>
>> -static void fimd_apply(void *in_ctx)
>> -{
>> - struct fimd_context *ctx = in_ctx;
>> - struct exynos_drm_manager *mgr = ctx->subdrv.manager;
>> - struct exynos_drm_manager_ops *mgr_ops = mgr->ops;
>> - struct fimd_win_data *win_data;
>> - int i;
>> -
>> - for (i = 0; i < WINDOWS_NR; i++) {
>> - win_data = &ctx->win_data[i];
>> - if (win_data->enabled && (mgr_ops && mgr_ops->win_commit))
>> - mgr_ops->win_commit(ctx, i);
>> - }
>> -
>> - if (mgr_ops && mgr_ops->commit)
>> - mgr_ops->commit(ctx);
>> -}
>> -
>> static void fimd_commit(void *in_ctx)
>> {
>> struct fimd_context *ctx = in_ctx;
>> @@ -597,6 +614,21 @@ static void fimd_commit(void *in_ctx)
>> writel(val, ctx->regs + VIDCON0);
>> }
>>
>> +static void fimd_apply(void *in_ctx)
>> +{
>> + struct fimd_context *ctx = in_ctx;
>> + struct fimd_win_data *win_data;
>> + int i;
>> +
>> + for (i = 0; i < WINDOWS_NR; i++) {
>> + win_data = &ctx->win_data[i];
>> + if (win_data->enabled)
>> + fimd_win_commit(ctx, i);
Implement fimd_win_enable function and call it at here,
if (win_data->enabled) {
fimd_win_commit(ctx,i);
fimd_win_enable(ctx, i); <- here
}
And, the below codes, for enable overlay dma channel, should be
removed from fimd_win_commit function,
/* wincon */
val = readl(ctx->regs + WINCON(win));
val |= WINCONx_ENWIN;
writel(val, ctx->regs + WINCON(win));
And, with [PATCH v2] drm/exynos: fimd: clean up pm suspend/resume, it
must work well.
>> + }
>> +
>> + fimd_commit(ctx);
>> +}
>> +
>> static int fimd_enable_vblank(void *in_ctx)
>> {
>> struct fimd_context *ctx = in_ctx;
>> @@ -661,6 +693,7 @@ static void fimd_wait_for_vblank(void *in_ctx)
>>
>> static struct exynos_drm_manager_ops fimd_manager_ops = {
>> .initialize = fimd_mgr_initialize,
>> + .remove = fimd_mgr_remove,
>> .dpms = fimd_dpms,
>> .apply = fimd_apply,
>> .commit = fimd_commit,
>> @@ -673,16 +706,13 @@ static struct exynos_drm_manager_ops fimd_manager_ops = {
>> };
>>
>> static struct exynos_drm_manager fimd_manager = {
>> - .pipe = -1,
>> - .ops = &fimd_manager_ops,
>> - .display_ops = &fimd_display_ops,
>> + .type = EXYNOS_DISPLAY_TYPE_LCD,
>> + .ops = &fimd_manager_ops,
>> };
>>
>> static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
>> {
>> struct fimd_context *ctx = (struct fimd_context *)dev_id;
>> - struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
>> - struct exynos_drm_manager *manager = subdrv->manager;
>> u32 val;
>>
>> val = readl(ctx->regs + VIDINTCON1);
>> @@ -692,11 +722,11 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
>> writel(VIDINTCON1_INT_FRAME, ctx->regs + VIDINTCON1);
>>
>> /* check the crtc is detached already from encoder */
>> - if (manager->pipe < 0 || !ctx->drm_dev)
>> + if (ctx->pipe < 0 || !ctx->drm_dev)
>> goto out;
>>
>> - drm_handle_vblank(ctx->drm_dev, manager->pipe);
>> - exynos_drm_crtc_finish_pageflip(ctx->drm_dev, manager->pipe);
>> + drm_handle_vblank(ctx->drm_dev, ctx->pipe);
>> + exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>>
>> /* set wait vsync event to zero and wake up queue. */
>> if (atomic_read(&ctx->wait_vsync_event)) {
>> @@ -707,39 +737,6 @@ out:
>> return IRQ_HANDLED;
>> }
>>
>> -static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev)
>> -{
>> - /*
>> - * enable drm irq mode.
>> - * - with irq_enabled = 1, we can use the vblank feature.
>> - *
>> - * P.S. note that we wouldn't use drm irq handler but
>> - * just specific driver own one instead because
>> - * drm framework supports only one irq handler.
>> - */
>> - drm_dev->irq_enabled = 1;
>> -
>> - /*
>> - * with vblank_disable_allowed = 1, vblank interrupt will be disabled
>> - * by drm timer once a current process gives up ownership of
>> - * vblank event.(after drm_vblank_put function is called)
>> - */
>> - drm_dev->vblank_disable_allowed = 1;
>> -
>> - /* attach this sub driver to iommu mapping if supported. */
>> - if (is_drm_iommu_supported(drm_dev))
>> - drm_iommu_attach_device(drm_dev, dev);
>> -
>> - return 0;
>> -}
>> -
>> -static void fimd_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
>> -{
>> - /* detach this sub driver from iommu mapping if supported. */
>> - if (is_drm_iommu_supported(drm_dev))
>> - drm_iommu_detach_device(drm_dev, dev);
>> -}
>> -
>> static int fimd_configure_clocks(struct fimd_context *ctx, struct device *dev)
>> {
>> struct videomode *vm = &ctx->panel.vm;
>> @@ -825,9 +822,8 @@ static int fimd_clock(struct fimd_context *ctx, bool enable)
>> return 0;
>> }
>>
>> -static void fimd_window_suspend(struct device *dev)
>> +static void fimd_window_suspend(struct fimd_context *ctx)
>> {
>> - struct fimd_context *ctx = get_fimd_context(dev);
>> struct fimd_win_data *win_data;
>> int i;
>>
>> @@ -839,9 +835,8 @@ static void fimd_window_suspend(struct device *dev)
>> fimd_wait_for_vblank(ctx);
>> }
>>
>> -static void fimd_window_resume(struct device *dev)
>> +static void fimd_window_resume(struct fimd_context *ctx)
>> {
>> - struct fimd_context *ctx = get_fimd_context(dev);
>> struct fimd_win_data *win_data;
>> int i;
>>
>> @@ -854,7 +849,6 @@ static void fimd_window_resume(struct device *dev)
>>
>> static int fimd_activate(struct fimd_context *ctx, bool enable)
>> {
>> - struct device *dev = ctx->subdrv.dev;
>> if (enable) {
>> int ret;
>>
>> @@ -866,11 +860,11 @@ static int fimd_activate(struct fimd_context *ctx, bool enable)
>>
>> /* if vblank was enabled status, enable it again. */
>> if (test_and_clear_bit(0, &ctx->irq_flags))
>> - fimd_enable_vblank(dev);
>> + fimd_enable_vblank(ctx);
>>
>> - fimd_window_resume(dev);
>> + fimd_window_resume(ctx);
>> } else {
>> - fimd_window_suspend(dev);
>> + fimd_window_suspend(ctx);
>>
>> fimd_clock(ctx, false);
>> ctx->suspended = true;
>> @@ -907,7 +901,6 @@ static int fimd_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct fimd_context *ctx;
>> - struct exynos_drm_subdrv *subdrv;
>> struct resource *res;
>> int win;
>> int ret = -EINVAL;
>> @@ -952,15 +945,6 @@ static int fimd_probe(struct platform_device *pdev)
>> DRM_INIT_WAITQUEUE(&ctx->wait_vsync_queue);
>> atomic_set(&ctx->wait_vsync_event, 0);
>>
>> - fimd_manager.ctx = ctx;
>> -
>> - subdrv = &ctx->subdrv;
>> -
>> - subdrv->dev = dev;
>> - subdrv->manager = &fimd_manager;
>> - subdrv->probe = fimd_subdrv_probe;
>> - subdrv->remove = fimd_subdrv_remove;
>> -
>> mutex_init(&ctx->lock);
>>
>> platform_set_drvdata(pdev, ctx);
>> @@ -971,7 +955,11 @@ static int fimd_probe(struct platform_device *pdev)
>> for (win = 0; win < WINDOWS_NR; win++)
>> fimd_clear_win(ctx, win);
>>
>> - exynos_drm_subdrv_register(subdrv);
>> + fimd_manager.ctx = ctx;
>> + exynos_drm_manager_register(&fimd_manager);
>> +
>> + fimd_display.ctx = ctx;
>> + exynos_drm_display_register(&fimd_display);
>>
>> return 0;
>> }
>> @@ -981,7 +969,8 @@ static int fimd_remove(struct platform_device *pdev)
>> struct device *dev = &pdev->dev;
>> struct fimd_context *ctx = platform_get_drvdata(pdev);
>>
>> - exynos_drm_subdrv_unregister(&ctx->subdrv);
>> + exynos_drm_display_unregister(&fimd_display);
>> + exynos_drm_manager_unregister(&fimd_manager);
>>
>> if (ctx->suspended)
>> goto out;
More information about the dri-devel
mailing list