On Tue, Oct 15, 2013 at 12:09 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:
> 2013/10/15 Inki Dae <inki.dae@xxxxxxxxxxx>:
>>> 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));
>
I'm not sure I follow why you want this done here. It seems unrelated
to this patch.
>
> And, with [PATCH v2] drm/exynos: fimd: clean up pm suspend/resume, it
> must work well.
I've got a patch to remove all of the
suspend/resume/pm_suspend/pm_resume stuff from all of the individual
drivers and do everything through dpms. This will eliminate the error
modes where the dpms state in the upper layers get out of sync with
the actual state of the hardware. It's also more consistent with other
drm drivers. I'll post it after this set.
Sean
>
>>> + }
>>> +
>>> + 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;
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
