| Message ID | 20240829-sm8650-v6-11-hmd-pocf-mdss-quad-upstream-8-v1-2-bdb05b4b5a2e@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Support quad pipe with dual-DSI | expand |
On Thu, 29 Aug 2024 at 13:19, Jun Nie <jun.nie@linaro.org> wrote: > > From: Jonathan Marek <jonathan@marek.ca> > > For the bonded DSI case, DSC pic_width and timing calculations should use > the width of a single panel instead of the total combined width. What is a "single panel"? Please rephrase the commit message so that it reads logically. > > Bonded DSI can be used to drive a single panel having two input > channels, or to drive two panels with a input channel on every panel that > behave like single panel for display controller. Missing actual action. See Documentation/process/submitting-patches.rst > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> As pointed out during internal review, missing Fixes tag. Any reason for ignoring it? > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- > drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +++++- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > index 87496db203d6c..35b90c462f637 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi.h > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > @@ -79,7 +79,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host); > int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > const struct drm_display_mode *mode); > enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > - const struct drm_display_mode *mode); > + const struct drm_display_mode *mode, > + bool is_bonded_dsi); > unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host); > int msm_dsi_host_register(struct mipi_dsi_host *host); > void msm_dsi_host_unregister(struct mipi_dsi_host *host); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 6388bb12696ff..7a4d9c071be5a 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -2489,7 +2489,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > } > > enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > - const struct drm_display_mode *mode) > + const struct drm_display_mode *mode, > + bool is_bonded_dsi) > { > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > struct drm_dsc_config *dsc = msm_host->dsc; > @@ -2499,6 +2500,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > if (!msm_host->dsc) > return MODE_OK; > > + if (is_bonded_dsi) > + pic_width = mode->hdisplay / 2; > + > if (pic_width % dsc->slice_width) { > pr_err("DSI: pic_width %d has to be multiple of slice %d\n", > pic_width, dsc->slice_width); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index a210b7c9e5ca2..6e915b57e14bb 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -420,7 +420,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, > return MODE_ERROR; > } > > - return msm_dsi_host_check_dsc(host, mode); > + return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI()); > } > > static int dsi_mgr_bridge_attach(struct drm_bridge *bridge, > > -- > 2.34.1 >
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年8月29日周四 18:54写道： > > On Thu, 29 Aug 2024 at 13:19, Jun Nie <jun.nie@linaro.org> wrote: > > > > From: Jonathan Marek <jonathan@marek.ca> > > > > For the bonded DSI case, DSC pic_width and timing calculations should use > > the width of a single panel instead of the total combined width. > > What is a "single panel"? Please rephrase the commit message so that > it reads logically. Yeah, it is a bit confusing without the usage case explanation. "single DSI interface" shall be much better here. Because bonded-DSI can work with a single panel with 2 DSI, or with 2 panels with one DSI on each panel. > > > > > Bonded DSI can be used to drive a single panel having two input > > channels, or to drive two panels with a input channel on every panel that > > behave like single panel for display controller. > > Missing actual action. See Documentation/process/submitting-patches.rst > > > > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > > As pointed out during internal review, missing Fixes tag. Any reason > for ignoring it? Sorry, it is missed by mistaken. I am just more familiar with "Signed-off-by" than "Fixes:" tag, so not sensitive to Fixed tag and miss it when you mention it. Will add it.
On Tue, 3 Sept 2024 at 10:32, Jun Nie <jun.nie@linaro.org> wrote: > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年8月29日周四 18:54写道： > > > > On Thu, 29 Aug 2024 at 13:19, Jun Nie <jun.nie@linaro.org> wrote: > > > > > > From: Jonathan Marek <jonathan@marek.ca> > > > > > > For the bonded DSI case, DSC pic_width and timing calculations should use > > > the width of a single panel instead of the total combined width. > > > > What is a "single panel"? Please rephrase the commit message so that > > it reads logically. > > Yeah, it is a bit confusing without the usage case explanation. "single DSI > interface" shall be much better here. Because bonded-DSI can work with > a single panel with 2 DSI, or with 2 panels with one DSI on each panel. Yes, it sounds much better. > > > > > > > > Bonded DSI can be used to drive a single panel having two input > > > channels, or to drive two panels with a input channel on every panel that > > > behave like single panel for display controller. > > > > Missing actual action. See Documentation/process/submitting-patches.rst > > > > > > > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > > > > As pointed out during internal review, missing Fixes tag. Any reason > > for ignoring it? > > Sorry, it is missed by mistaken. I am just more familiar with > "Signed-off-by" than > "Fixes:" tag, so not sensitive to Fixed tag and miss it when you > mention it. Will > add it. Well, the regular rule of reviews applies: if you don't agree or don't understand, please reply first.
On 2024-08-29 18:17:31, Jun Nie wrote: > From: Jonathan Marek <jonathan@marek.ca> > > For the bonded DSI case, DSC pic_width and timing calculations should use > the width of a single panel instead of the total combined width. When this patch was originally proposed we already discussed [1] that this is **not** universally true. On my hardware a single bonded panel always receives the full width, at least on downstream kernels, and it works [2]. [1]: https://lore.kernel.org/linux-arm-msm/eanx45rnasj7lu3r2tfhtg4qkqkcidd6zctsz6ci6jlklu4fgi@3nf73w2ka4li/T/#u [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/41 Can we please figure this out before landing this patch? - Marijn > Bonded DSI can be used to drive a single panel having two input > channels, or to drive two panels with a input channel on every panel that > behave like single panel for display controller. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- > drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +++++- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > index 87496db203d6c..35b90c462f637 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi.h > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > @@ -79,7 +79,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host); > int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > const struct drm_display_mode *mode); > enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > - const struct drm_display_mode *mode); > + const struct drm_display_mode *mode, > + bool is_bonded_dsi); > unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host); > int msm_dsi_host_register(struct mipi_dsi_host *host); > void msm_dsi_host_unregister(struct mipi_dsi_host *host); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 6388bb12696ff..7a4d9c071be5a 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -2489,7 +2489,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > } > > enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > - const struct drm_display_mode *mode) > + const struct drm_display_mode *mode, > + bool is_bonded_dsi) > { > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > struct drm_dsc_config *dsc = msm_host->dsc; > @@ -2499,6 +2500,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, > if (!msm_host->dsc) > return MODE_OK; > > + if (is_bonded_dsi) > + pic_width = mode->hdisplay / 2; > + > if (pic_width % dsc->slice_width) { > pr_err("DSI: pic_width %d has to be multiple of slice %d\n", > pic_width, dsc->slice_width); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index a210b7c9e5ca2..6e915b57e14bb 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -420,7 +420,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, > return MODE_ERROR; > } > > - return msm_dsi_host_check_dsc(host, mode); > + return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI()); > } > > static int dsi_mgr_bridge_attach(struct drm_bridge *bridge, > > -- > 2.34.1 >
On 2024-09-03 11:50:36, Marijn Suijten wrote: > On 2024-08-29 18:17:31, Jun Nie wrote: > > From: Jonathan Marek <jonathan@marek.ca> > > > > For the bonded DSI case, DSC pic_width and timing calculations should use > > the width of a single panel instead of the total combined width. > > When this patch was originally proposed we already discussed [1] that this is > **not** universally true. On my hardware a single bonded panel always receives > the full width, at least on downstream kernels, and it works [2]. > > [1]: https://lore.kernel.org/linux-arm-msm/eanx45rnasj7lu3r2tfhtg4qkqkcidd6zctsz6ci6jlklu4fgi@3nf73w2ka4li/T/#u > [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/41 > > Can we please figure this out before landing this patch? For completeness I've picked this patch, together with the following mis-squashed change from patch 03/21: diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 7a4d9c071be5a..5abade8f26b88 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -953,7 +953,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) return; } - dsc->pic_width = mode->hdisplay; + dsc->pic_width = hdisplay; dsc->pic_height = mode->vdisplay; DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); And this is what it looks like on a bonded DSI CMD-mode display: https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2553207 https://gitlab.freedesktop.org/-/project/2206/uploads/dc5c53d09ecb635fdc9f190fbc9b37ac/1000027079.jpg That's a clear regression :) - Marijn
Marijn Suijten <marijn.suijten@somainline.org> 于2024年9月3日周二 19:51写道： > > On 2024-09-03 11:50:36, Marijn Suijten wrote: > > On 2024-08-29 18:17:31, Jun Nie wrote: > > > From: Jonathan Marek <jonathan@marek.ca> > > > > > > For the bonded DSI case, DSC pic_width and timing calculations should use > > > the width of a single panel instead of the total combined width. > > > > When this patch was originally proposed we already discussed [1] that this is > > **not** universally true. On my hardware a single bonded panel always receives > > the full width, at least on downstream kernels, and it works [2]. > > > > [1]: https://lore.kernel.org/linux-arm-msm/eanx45rnasj7lu3r2tfhtg4qkqkcidd6zctsz6ci6jlklu4fgi@3nf73w2ka4li/T/#u > > [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/41 > > > > Can we please figure this out before landing this patch? > > For completeness I've picked this patch, together with the following > mis-squashed change from patch 03/21: > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 7a4d9c071be5a..5abade8f26b88 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -953,7 +953,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > return; > } > > - dsc->pic_width = mode->hdisplay; > + dsc->pic_width = hdisplay; > dsc->pic_height = mode->vdisplay; > DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); > > And this is what it looks like on a bonded DSI CMD-mode display: > https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2553207 > https://gitlab.freedesktop.org/-/project/2206/uploads/dc5c53d09ecb635fdc9f190fbc9b37ac/1000027079.jpg > > That's a clear regression :) > > - Marijn Surely we should figure out what's going on here before we land this patch. Please help me to understand your setup first. Your panel works well on mainline kernel with 2:2:2(?) topology, and you see issue with applying the change here, right? There are several parameter that impact the issue, video/command DSI mode, single panel with 2 DSI vs 2 panels with single DSI on each panel, 4:4:2 topology on my device vs your topology. Let's list all of them before further discussion. -Jun
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 87496db203d6c..35b90c462f637 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -79,7 +79,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host); int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, const struct drm_display_mode *mode); enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, - const struct drm_display_mode *mode); + const struct drm_display_mode *mode, + bool is_bonded_dsi); unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host); int msm_dsi_host_register(struct mipi_dsi_host *host); void msm_dsi_host_unregister(struct mipi_dsi_host *host); diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 6388bb12696ff..7a4d9c071be5a 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2489,7 +2489,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, } enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, - const struct drm_display_mode *mode) + const struct drm_display_mode *mode, + bool is_bonded_dsi) { struct msm_dsi_host *msm_host = to_msm_dsi_host(host); struct drm_dsc_config *dsc = msm_host->dsc; @@ -2499,6 +2500,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host, if (!msm_host->dsc) return MODE_OK; + if (is_bonded_dsi) + pic_width = mode->hdisplay / 2; + if (pic_width % dsc->slice_width) { pr_err("DSI: pic_width %d has to be multiple of slice %d\n", pic_width, dsc->slice_width); diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index a210b7c9e5ca2..6e915b57e14bb 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -420,7 +420,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, return MODE_ERROR; } - return msm_dsi_host_check_dsc(host, mode); + return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI()); } static int dsi_mgr_bridge_attach(struct drm_bridge *bridge,