2024-01-26 18:36:39

by Dave Airlie

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

Just FYI this conflictted pretty heavily with drm-misc-next changes in
the same area, someone should check drm-tip has the correct
resolution, I'm not really sure what is definitely should be.

Dave.

On Fri, 19 Jan 2024 at 16:37, Inki Dae <[email protected]> wrote:
>
> Really sorry for late. Will pick it up.
>
> Thanks,
> Inki Dae
>
> 2024년 1월 9일 (화) 오후 9:50, Daniel Vetter <[email protected]>님이 작성:
>>
>> On Tue, Jan 09, 2024 at 09:47:20AM +0100, Michael Walle wrote:
>> > Hi,
>> >
>> > > > Inki, are you picking this up? Or if not, who will?
>> > >
>> > > I can pick it up but it would be better to go to the drm-misc tree. If
>> > > nobody cares about it then I will pick it up. :)
>> > >
>> > > acked-by : Inki Dae <[email protected]>
>> >
>> > Who is going to pick this up? Who has access to the drm-misc tree?
>>
>> Inki has, so I'm assuming since he acked he'll also merge.
>> -Sima
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch


2024-01-29 09:20:45

by Frieder Schrempf

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

On 26.01.24 19:28, Dave Airlie wrote:
> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> the same area, someone should check drm-tip has the correct
> resolution, I'm not really sure what is definitely should be.
>
> Dave.

Thanks! I took a quick look at what is now in Linus' tree and it looks
correct to me. The only thing I'm missing is my Reviewed-by tag which
got lost somewhere, but I can get over that.

2024-01-29 10:32:58

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> the same area, someone should check drm-tip has the correct
> resolution, I'm not really sure what is definitely should be.

FWIW, this looks rather messy now. The drm-tip doesn't build.

There was a new call to samsung_dsim_set_stop_state() introduced
in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
mode in the enable() callback).

Also merge commit 663a907e199b (Merge remote-tracking branch
'drm-misc/drm-misc-next' into drm-tip) is broken because it
completely removes samsung_dsim_atomic_disable(). Dunno whats
going on there.

I'm just about to look at getting it to compile again and
I'm trying to retest it.

-michael

2024-01-29 10:40:03

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

> Also merge commit 663a907e199b (Merge remote-tracking branch
> 'drm-misc/drm-misc-next' into drm-tip) is broken because it
> completely removes samsung_dsim_atomic_disable(). Dunno whats
> going on there.

Actually, that merge commit looks even worse. It somehow folds
the original samsung_dsim_atomic_disable() into
samsung_dsim_atomic_enable(). Therefore, now the enable op
will clear the DSIM_STATE_VIDOUT_AVAILABLE flag?! It will also
never be set. Dunno how to proceed here.

-michael

2024-01-29 16:06:40

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

>> Just FYI this conflictted pretty heavily with drm-misc-next changes in
>> the same area, someone should check drm-tip has the correct
>> resolution, I'm not really sure what is definitely should be.
>
> FWIW, this looks rather messy now. The drm-tip doesn't build.
>
> There was a new call to samsung_dsim_set_stop_state() introduced
> in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
> mode in the enable() callback).

I had a closer look at the latest linux-next (where somehow my patch
made it into) and tried to apply commit b2fe2292624ac (drm: bridge:
samsung-dsim: enter display mode in the enable() callback). It looks
like only the following hunk is still needed from that patch. Everything
else is covered by this fixes patch.

Dario, could you rebase your commit onto this patch? I had a quick test
with this change and it seems to work fine for our case.

--snip--
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 63a1a0c88be4..92755c90e7d2 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1498,6 +1498,8 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
if (!(dsi->state & DSIM_STATE_ENABLED))
return;

+ samsung_dsim_set_display_enable(dsi, false);
+
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}

@@ -1506,8 +1508,6 @@ static void
samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);

- samsung_dsim_set_display_enable(dsi, false);
-
dsi->state &= ~DSIM_STATE_ENABLED;
pm_runtime_put_sync(dsi->dev);
}
--snip--

-michael

2024-01-29 16:51:28

by Frieder Schrempf

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

On 29.01.24 10:20, Frieder Schrempf wrote:
> On 26.01.24 19:28, Dave Airlie wrote:
>> Just FYI this conflictted pretty heavily with drm-misc-next changes in
>> the same area, someone should check drm-tip has the correct
>> resolution, I'm not really sure what is definitely should be.
>>
>> Dave.
>
> Thanks! I took a quick look at what is now in Linus' tree and it looks
> correct to me. The only thing I'm missing is my Reviewed-by tag which
> got lost somewhere, but I can get over that.

Apparently I missed the point here. I was looking at the wrong trees
(drm-next and master instead of drm-misc-next and drm-tip). Sorry for
the noise. Michael already pointed out the correct details.

2024-01-30 09:39:08

by Dario Binacchi

[permalink] [raw]
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

Hi Michael,

On Mon, Jan 29, 2024 at 5:06 PM Michael Walle <[email protected]> wrote:
>
> >> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> >> the same area, someone should check drm-tip has the correct
> >> resolution, I'm not really sure what is definitely should be.
> >
> > FWIW, this looks rather messy now. The drm-tip doesn't build.
> >
> > There was a new call to samsung_dsim_set_stop_state() introduced
> > in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
> > mode in the enable() callback).
>
> I had a closer look at the latest linux-next (where somehow my patch
> made it into) and tried to apply commit b2fe2292624ac (drm: bridge:
> samsung-dsim: enter display mode in the enable() callback). It looks
> like only the following hunk is still needed from that patch. Everything
> else is covered by this fixes patch.
>
> Dario, could you rebase your commit onto this patch? I had a quick test
> with this change and it seems to work fine for our case.
>
> --snip--
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 63a1a0c88be4..92755c90e7d2 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1498,6 +1498,8 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge,
> if (!(dsi->state & DSIM_STATE_ENABLED))
> return;
>
> + samsung_dsim_set_display_enable(dsi, false);
> +
> dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> }
>
> @@ -1506,8 +1508,6 @@ static void
> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> {
> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>
> - samsung_dsim_set_display_enable(dsi, false);
> -
> dsi->state &= ~DSIM_STATE_ENABLED;
> pm_runtime_put_sync(dsi->dev);
> }
> --snip--
>
> -michael

I'm sorry, but I didn't understand well what I have to do.
This is what I have done:

git clone https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next.git
cd linux-next
# add your changes, the ones of the emails
git am --reject 0001-drm-bridge-samsung-dsim-enter-display-mode-in-the-en.patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 92755c90e7d2..b47929072583 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1508,6 +1508,9 @@ static void
samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);

+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+ samsung_dsim_set_stop_state(dsi, true);
+
dsi->state &= ~DSIM_STATE_ENABLED;
pm_runtime_put_sync(dsi->dev);
}

And then test the driver for my use case.

Is everything I wrote correct, or am I making a mistake?

Thanks and regards,
Dario

--

Dario Binacchi

Senior Embedded Linux Developer

[email protected]

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
[email protected]

http://www.amarulasolutions.com