Re: [Freedreno] [PATCH] drm/msm/dpu: always program dsc active bits

[Abhinav Kumar]

On 4/14/2023 4:11 PM, Marijn Suijten wrote:
> On 2023-04-14 10:57:45, Abhinav Kumar wrote:
> > On 4/14/2023 10:34 AM, Marijn Suijten wrote:
> > > On 2023-04-14 08:48:43, Abhinav Kumar wrote:
> > > > On 4/14/2023 12:35 AM, Marijn Suijten wrote:
> > > > > On 2023-04-12 10:33:15, Abhinav Kumar wrote:
> > > > > [..]
> > > > > > > What happens if a device boots without DSC panel connected?  Will
> > > > > > > CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the
> > > > > > > DSC blocks?  Or could this flush uninitialized state to the block?
> > > > > >
> > > > > > If we bootup without DSC panel connected, the kernel's cfg->dsc will be
> > > > > > 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush
> > > > > > any DSC blocks.
> > > > >
> > > > > Ack, that makes sense.  However, if I connect a DSC panel, then
> > > > > disconnect it (now the register should be non-zero, but cfg->dsc will be
> > > > > zero), and then replug a non-DSC panel multiple times, it'll get flushed
> > > > > every time because we never clear CTL_DSC_FLUSH after that?
> > > >
> > > > If we remove it after kernel starts, that issue is there even today
> > > > without that change because DSI is not a hot-pluggable display so a
> > > > teardown wont happen when you plug out the panel. How will cfg->dsc be 0
> > > > then? In that case, its not a valid test as there was no indication to
> > > > DRM that display was disconnected so we cannot tear it down.
> > >
> > > The patch description itself describes hot-pluggable displays, which I
> > > believe is the upcoming DSC support for DP?  You ask how cfg->dsc can
> > > become zero, but this is **exactly** what the patch description
> > > describes, and what this patch is removing the `if` for.  If we are not
> > > allowed to discuss that scenario because it is not currently supported,
> > > neither should we allow to apply this patch.
> > >
> > > With that in mind, can you re-answer the question?
> >
> > I didnt follow what needs to be re-answered.
> >
> > This patch is being sent in preparation of the DSC over DP support. This
> > does not handle non-hotpluggable displays.
>
> Good, because my question is specifically about *hotpluggable*
> displays/panels like the upcoming DSC support for DP.  After all there
> would be no point in me suggesting to connect and disconnect
> non-hotpluggable displays and expect something sensible to happen,
> wouldn't it?  Allow me to copy-paste the question again for convenience,
> with some minor wording changes:
>
> 	However, if I connect a DSC DP display, then disconnect it (now the
> 	register should be non-zero, but cfg->dsc will be zero), and then
> 	connect and reconnect a non-DSC DP display multiple times, it'll get
> 	flushed every time because we never clear CTL_DSC_FLUSH after that?
>
> And the missing part is: would multiple flushes be harmful in this case?

Well, you kept asking about "DSC panel" , that made me think you were 
asking about a non-hotpluggable MIPI DSI DSC panel and not DP DSC 
monitor. On many boards, panels can be removed/connected back to their 
daughter card. The term "panel" confused me a bit.

Now answering your question.

Yes, it will get flushed once every hotplug thats not too bad but 
importantly DSC wont be active as CTL_DSC_ACTIVE will be set to 0 so it 
wont cause any issue.


> > I do not think dynamic switch
> > between DSC and non-DSC of non-hotpluggable displays needs to be
> > discussed here as its not handled at all with or without this patch.
> >
> > We wanted to get early reviews on the patch. If you want this patch to
> > be absorbed when rest of DSC over DP lands, I have no concerns with
> > that. I wont pick this up for fixes and we will land this together with
> > the rest of DP over DSC.
>
> I don't mind when and where this lands, just want to have the semantics
> clear around persisting the value of CTL_DSC_FLUSh in the register.
>
> Regardless, this patch doesn't sound like a fix but a workaround until
> reset_intf_cfg() is fixed to be called at the right point, and extended
> to clear CTL_DSC_ACTIVE and flush the DSCs.  Perhaps it shouldn't have a
> Fixes: tag for that reason, as you intend to reinstate this
> if (cfg->dsc) condition when that is done?

Its certainly fixing the use-case of DSC to non-DSC switching. So it is 
a fix.

But yes not the best fix possible. We have to improve it by moving this 
to reset_intf_cfg() as I already committed to.


> - Marijn