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
