Re: [PATCH i-g-t v1 0/3] Revert HDCP per crtc infrastructure

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

> > 

[snip]

> > > Hi Santhosh,
> > > 
> > 
> > Hi Jason,
> > 
> > > It seems you and I have a conflicting issue:
> > > [1] Undersized buffers: When 4K output uses CRTC with 2K FB [2]
> > > Oversized
> > > buffers: When 3504x2190 panel would get 3840x2160 FB
> > > 
> > > [1] that requires reverting to maximum resolution, which in turn
> > > causes my test scenario [2] to regress.
> > > 
> > > Therefore, I think we should find a solution that satisfies both
> > > needs, rather than simply reverting my entire series.
> > > 
> > > 
> > > Here is my solution:
> > > Dynamically check and recreate framebuffers in modeset_with_fb():
> > > - If existing FB is large enough → reuse it
> > > - If existing FB is too small → remove and recreate with required
> > > size
> > > - Only allocate what's actually needed, when needed
> > > 
> > > This provides optimal memory usage while preventing both
> > > undersized
> > > and oversized buffer issues.
> > > 
> > 
> > To point out [2] isn’t that the buffer is oversized its actually
> > that the buffer that
> > would use maximum size.
> > We end up creating a buffer with maximum size of 3840 x2190 this is
> > then
> > clamped to the correct size by igt_fb_set_size() In
> > modeset_with_fb(). The
> > problem from what I can see is that it does not do
> > igt_plane_set_size().
> > 
> > Which is lacking in some other places too and I think that should
> > have been
> > the straightforward and simple fix for [2] .
> > Maybe you can try than and see if that alone fixes your issue
> > without such a
> > big refactor.
> > Santhosh you can test that too from your side and if it works add a
> > patch this
> > in revert series so the [2] does not regress.
> > 
> > Also was  a gitlab issue for this created before a refactor series
> > for this was
> > sent ?
> > That would have been the correct way to go about it so that the
> > initial debug
> > could be a joint one to  avoid such situations.
> > 
> > 
> > Moreover this series is now creating fbs for each connected output
> > - for each
> > crtc running allocation num_outputs * num_crtc times cleanup is
> > just running
> > the cleanup num_crtc  times.
> > 
> > Also creating FB for each CRTC does not make sense since we only
> > really ever
> > use the first CRTC for testing purposes breaking out of loop once
> > everything
> > passes on CRTC.
> > Only in MST cases do we end up using more than one CRTC. But now we
> > are
> > essentially creating more FB's than required and then cleaning them
> > up adding
> > extra work and load while test is running.
> > 
> > To make it dynamic could have made much more sense to create FB for
> > each
> > connected output. So yes definitely the whole test requires rework
> > but this
> > refactor is taking is further from the goal of having this
> > optimized.
> > More over in MST side of things these tests are getting it to
> > skipped
> > altogether.
> > 
> > This whole series has regressed our Pass rate quite significantly,
> > without
> > catching any real kernel issues. Trying to fix on top of this will
> > require a lot of
> > hotfixes breaking one thing after another. Increasing our Techincal
> > debt and
> > the effort one would require to really optimize this.
> > To reiterate yes, this test absolutely can do a better job at
> > memory
> > management along with some other refactors. But we really will have
> > to a
> > take a step back and look at the big picture before really doing
> > big refactors
> > here or just hot fixing on the current refactors.
> > 
> > Also the find  minimal change that may fix [2] event after
> > reverting the 3
> > patches. Santhosh can you try this please. Jason you can Also have
> > a go .
> 
> Also just use this series to apply will make testing easier 
> https://urldefense.com/v3/__https://patchwork.freedesktop.org/series/166614/__;!!CTRNKA9wMg0ARbw!n9_RHyzncqZuErA5zDPoC-wSNRcUC9J6Ow86AtegeS6Mj8Cnjzpt17TnjUYQlhbuJXMblqDuWISX5DzjqmJq1gQJ5Nqq$
>  
> 
> Also for some reason patchworks shows I submitted this series no idea
> why ?
> 
> Regards,
> Suraj Kandpal
> 
> 

Hi Suraj,

Thank you for the detailed review and the thoughtful analysis.

You're right that simply reverting the series may not be the cleanest
path forward, and I appreciate you taking the time to identify the
root cause of [2] and proposing a minimal fix.

However, if the regression impact on existing tests turns out to be
significant, I think reverting first would still be the safer option
for now.
That said, I would really appreciate it if, when I send patches in the
future, you could help verify them in the same test environment to
catch any potential regressions early. That kind of collaboration
would go a long way in preventing similar situations from happening
again.

Regarding the igt_plane_set_size() suggestion — that does look like a
straightforward and targeted fix for the oversized buffer issue [2].
I will try your patch:
- https://patchwork.freedesktop.org/patch/725245/
on my side and report back with the results.

Regarding the GitLab issue — I apologize, as I wasn't familiar with
this process. Additionally, whenever I visit link:
- https://gitlab.freedesktop.org/drm/igt-gpu-tools/
I keep seeing the message: "Due to an influx of spam, we have had to
impose restrictions on new accounts. Please see this wiki page
for instructions on how to get full permissions. Sorry for the
inconvenience." and showing nothing on the webpage.
Could you point me in the right direction on where and how I should
open an issue for discussion in the future? I want to make sure I
follow the correct process going forward to facilitate better
collaboration and avoid such situations.

Thanks again for the guidance and for taking the time to write such a
thorough review.

Best regards,
Jason-JH Lin