Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner <stefan@agner.ch>]

On 2017-11-14 16:40, Noralf Trønnes wrote:
> Den 14.11.2017 16.23, skrev Stefan Agner:
>> On 2017-11-10 19:06, Noralf Trønnes wrote:
>>> Den 10.11.2017 17.39, skrev Stefan Agner:
>>>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>>>> Replace driver's code with the generic helpers that do the same thing.
>>>>>> Tested using:
>>>>>> echo devices > /sys/power/pm_test
>>>>>> echo freeze > /sys/power/state
>>>>>>
>>>>>>
>>>>>> Note, currently I do get, but even without this patches, so this is
>>>>>> something else:
>>>>>>
>>>>>> [  930.992433] ------------[ cut here ]------------
>>>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>>>> I resolved that issue and another related to suspend/resume for the DCU
>>>> driver:
>>>> https://patchwork.freedesktop.org/series/33616/
>>>>
>>>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>>>> on, so that is why I did not catch this earlier.
>>>>
>>>> This two patches are now on-top of your changes. How can we make sure
>>>> this goes through smoothly? For which merge window are you targeting
>>>> your changes?
>>> drm-misc is always open so I'm planning to apply it this weekend or
>>> maybe monday.
>>> This means it will go into 4.16. Maybe you need to fix it before that?
>> The issues were already present in 4.14, so anyway to late for that. The
>> suspend/resume platform support for the SoC using this IP is missing in
>> mainline anyway, so in practice this patches are not that critical. I
>> probably will backport it for v4.14 since its LTS once it hits mainline.
> 
> In that case shouldn't you fix that issue first with fixes tag and cc stable?
> Then I can rebase this patch on top of that. This way we avoid merge conflict
> when your fix is backported to 4.14 and 4.15.
> 
> Or have I missed something here?

That is probably the right approach. Whatever causes the least amount of
friction.

Try to understand how to go about that:
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#where-do-i-apply-my-patch

Release already happened, but rc1 not yet, so I guess I should base on
drm-misc-next-fixes now, right?

So I prepare a drm-fsl-dcu-next fixes-forbranch, based on
drm-misc-next-fixes and send it to Dave.

Rebasing once it is merged rebasing should rather trivial.

--
Stefan


> 
> Noralf.
> 
> 
>> So from my point of view, 4.16 is fine.
>>
>> Should I create a separate pull request for those or can you pick them
>> up directly?
>>
>> --
>> Stefan
>>
>>
>>> Could you help me out by acking the tinydrm patch in this series?
>>>
>>> Noralf.
>>>
>>>> --
>>>> Stefan
>>>>
>>>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>>>
>>>>>> Will you take the patch through drm-misc?
>>>>> Yes if that's fine with you, thanks for testing.
>>>>>
>>>>> Noralf.
>>>>>
>>>>>> --
>>>>>> Stefan
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>>>     2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>     #include <drm/drm_crtc_helper.h>
>>>>>>>     #include <drm/drm_fb_cma_helper.h>
>>>>>>>     #include <drm/drm_gem_cma_helper.h>
>>>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>>>       #include "fsl_dcu_drm_crtc.h"
>>>>>>>     #include "fsl_dcu_drm_drv.h"
>>>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>>>     static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>>>     {
>>>>>>>     	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>>>> +	int ret;
>>>>>>>       	if (!fsl_dev)
>>>>>>>     		return 0;
>>>>>>>       	disable_irq(fsl_dev->irq);
>>>>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>>>     -	console_lock();
>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>>>> -	console_unlock();
>>>>>>> -
>>>>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>>>>> -		console_lock();
>>>>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>> -		console_unlock();
>>>>>>> -
>>>>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>>>> +	if (ret) {
>>>>>>>     		enable_irq(fsl_dev->irq);
>>>>>>> -		return PTR_ERR(fsl_dev->state);
>>>>>>> +		return ret;
>>>>>>>     	}
>>>>>>>       	clk_disable_unprepare(fsl_dev->pix_clk);
>>>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>>>>     	if (fsl_dev->tcon)
>>>>>>>     		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>>>     	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>>>     -	console_lock();
>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>> -	console_unlock();
>>>>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>>>     -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>>     	enable_irq(fsl_dev->irq);
>>>>>>>       	return 0;
>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>>>     	struct drm_encoder encoder;
>>>>>>>     	struct fsl_dcu_drm_connector connector;
>>>>>>>     	const struct fsl_dcu_soc_data *soc;
>>>>>>> -	struct drm_atomic_state *state;
>>>>>>>     };
>>>>>>>       int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel