2013/10/16 Sean Paul <seanpaul@xxxxxxxxxxxx>:
> 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.
>
Yes, it's unrelated to this patch. I mean additional work.
>>
>> 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.
>
It seems like more clear if we _can_do_ so. However, can the dpms be
called when entering sleep? And I think this way - removing all pm
related stuff from device drivers - is not reasonable, and we'd need
more discussion about that with pm people. So add the related patch
set at top of your re-factoring patch set if you want to do so.
Thanks,
Inki Dae
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
