| Message ID | 20240725220320.130916-1-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dp: fix the max supported bpp logic | expand |
Quoting Abhinav Kumar (2024-07-25 15:03:19) > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > index a916b5f3b317..56ce5e4008f8 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -423,8 +424,10 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) > drm_mode->clock); > drm_dbg_dp(panel->drm_dev, "bpp = %d\n", dp_panel->dp_mode.bpp); > > - dp_panel->dp_mode.bpp = max_t(u32, 18, > - min_t(u32, dp_panel->dp_mode.bpp, 30)); > + max_supported_bpp = dp_panel_get_mode_bpp(dp_panel, dp_panel->dp_mode.bpp, > + dp_panel->dp_mode.drm_mode.clock); > + dp_panel->dp_mode.bpp = max_t(u32, 18, max_supported_bpp); Is the max_t() usage still required once 'max_supported_bpp' is also a u32? Also, what is 18? Shouldn't that be some sort of define so we know what it represents? Or maybe none of that is required? From what I can tell, dp_panel_get_mode_bpp() calls dp_panel_get_supported_bpp() which will essentially clamp the bpp range between 18 and 30, unless dp_panel->dp_mode.bpp is between 30 and 18 but not divisible by 6, e.g. 29. Perhaps this patch can be included and the max_t above dropped. ---8<-- diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 07db8f37cd06..5cd7c138afd3 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -90,22 +90,22 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel) static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel, u32 mode_edid_bpp, u32 mode_pclk_khz) { - struct dp_link_info *link_info; + const struct dp_link_info *link_info; const u32 max_supported_bpp = 30, min_supported_bpp = 18; - u32 bpp = 0, data_rate_khz = 0; + u32 bpp, data_rate_khz; bpp = min_t(u32, mode_edid_bpp, max_supported_bpp); link_info = &dp_panel->link_info; data_rate_khz = link_info->num_lanes * link_info->rate * 8; - while (bpp > min_supported_bpp) { + do { if (mode_pclk_khz * bpp <= data_rate_khz) - break; + return bpp; bpp -= 6; - } + } while (bpp > min_supported_bpp); - return bpp; + return min_supported_bpp; } static int dp_panel_update_modes(struct drm_connector *connector,
On Fri, 26 Jul 2024 at 01:04, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Currently the DP driver hard-codes the max supported bpp to 30. > This is incorrect because the number of lanes and max data rate > supported by the lanes need to be taken into account. > > Replace the hardcoded limit with the appropriate math which accounts > for the accurate number of lanes and max data rate. > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_panel.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > index a916b5f3b317..56ce5e4008f8 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -397,6 +397,7 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) > { > struct drm_display_mode *drm_mode; > struct dp_panel_private *panel; > + u32 max_supported_bpp; > > drm_mode = &dp_panel->dp_mode.drm_mode; > > @@ -423,8 +424,10 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) > drm_mode->clock); > drm_dbg_dp(panel->drm_dev, "bpp = %d\n", dp_panel->dp_mode.bpp); > > - dp_panel->dp_mode.bpp = max_t(u32, 18, > - min_t(u32, dp_panel->dp_mode.bpp, 30)); > + max_supported_bpp = dp_panel_get_mode_bpp(dp_panel, dp_panel->dp_mode.bpp, > + dp_panel->dp_mode.drm_mode.clock); > + dp_panel->dp_mode.bpp = max_t(u32, 18, max_supported_bpp); I think that in mode_valid() the driver should filter out modes that result in BPP being less than 18. Then the max_t can be dropped completely. Nevertheless this indeed fixes an issue with the screen corruption, this is great! Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> # SM8350-HDK > + > drm_dbg_dp(panel->drm_dev, "updated bpp = %d\n", > dp_panel->dp_mode.bpp); > > -- > 2.44.0 >
Hi Stephen On 7/26/2024 5:24 PM, Stephen Boyd wrote: > Quoting Abhinav Kumar (2024-07-25 15:03:19) >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c >> index a916b5f3b317..56ce5e4008f8 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c >> @@ -423,8 +424,10 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) >> drm_mode->clock); >> drm_dbg_dp(panel->drm_dev, "bpp = %d\n", dp_panel->dp_mode.bpp); >> >> - dp_panel->dp_mode.bpp = max_t(u32, 18, >> - min_t(u32, dp_panel->dp_mode.bpp, 30)); >> + max_supported_bpp = dp_panel_get_mode_bpp(dp_panel, dp_panel->dp_mode.bpp, >> + dp_panel->dp_mode.drm_mode.clock); >> + dp_panel->dp_mode.bpp = max_t(u32, 18, max_supported_bpp); > > Is the max_t() usage still required once 'max_supported_bpp' is also a > u32? Also, what is 18? Shouldn't that be some sort of define so we know > what it represents? > > Or maybe none of that is required? From what I can tell, > dp_panel_get_mode_bpp() calls dp_panel_get_supported_bpp() which will > essentially clamp the bpp range between 18 and 30, unless > dp_panel->dp_mode.bpp is between 30 and 18 but not divisible by 6, e.g. > 29. Perhaps this patch can be included and the max_t above dropped. > > ---8<-- > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > b/drivers/gpu/drm/msm/dp/dp_panel.c > index 07db8f37cd06..5cd7c138afd3 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -90,22 +90,22 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel) > static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel, > u32 mode_edid_bpp, u32 mode_pclk_khz) > { > - struct dp_link_info *link_info; > + const struct dp_link_info *link_info; > const u32 max_supported_bpp = 30, min_supported_bpp = 18; > - u32 bpp = 0, data_rate_khz = 0; > + u32 bpp, data_rate_khz; > > bpp = min_t(u32, mode_edid_bpp, max_supported_bpp); > > link_info = &dp_panel->link_info; > data_rate_khz = link_info->num_lanes * link_info->rate * 8; > > - while (bpp > min_supported_bpp) { > + do { > if (mode_pclk_khz * bpp <= data_rate_khz) > - break; > + return bpp; > bpp -= 6; > - } > + } while (bpp > min_supported_bpp); > > - return bpp; > + return min_supported_bpp; > } > Thanks for the feedback. Your change looks valid. We can use this and drop the max_t usage. Let me push this with your Suggested-by credits. > static int dp_panel_update_modes(struct drm_connector *connector,
On 7/27/2024 5:51 AM, Dmitry Baryshkov wrote: > On Fri, 26 Jul 2024 at 01:04, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> Currently the DP driver hard-codes the max supported bpp to 30. >> This is incorrect because the number of lanes and max data rate >> supported by the lanes need to be taken into account. >> >> Replace the hardcoded limit with the appropriate math which accounts >> for the accurate number of lanes and max data rate. >> >> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_panel.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c >> index a916b5f3b317..56ce5e4008f8 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c >> @@ -397,6 +397,7 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) >> { >> struct drm_display_mode *drm_mode; >> struct dp_panel_private *panel; >> + u32 max_supported_bpp; >> >> drm_mode = &dp_panel->dp_mode.drm_mode; >> >> @@ -423,8 +424,10 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) >> drm_mode->clock); >> drm_dbg_dp(panel->drm_dev, "bpp = %d\n", dp_panel->dp_mode.bpp); >> >> - dp_panel->dp_mode.bpp = max_t(u32, 18, >> - min_t(u32, dp_panel->dp_mode.bpp, 30)); >> + max_supported_bpp = dp_panel_get_mode_bpp(dp_panel, dp_panel->dp_mode.bpp, >> + dp_panel->dp_mode.drm_mode.clock); >> + dp_panel->dp_mode.bpp = max_t(u32, 18, max_supported_bpp); > > I think that in mode_valid() the driver should filter out modes that > result in BPP being less than 18. Then the max_t can be dropped > completely. > With Stephen's suggested change, dp_panel_get_supported_bpp() will not return anything below min_supported_bpp which is 18 so we can absorb that part and drop the max_t part here. > Nevertheless this indeed fixes an issue with the screen corruption, > this is great! > > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> # SM8350-HDK > Thanks for reporting and testing this. I need to give you Reported-by credits as well. >> + >> drm_dbg_dp(panel->drm_dev, "updated bpp = %d\n", >> dp_panel->dp_mode.bpp); >> >> -- >> 2.44.0 >> > >
Quoting Abhinav Kumar (2024-07-29 11:28:35) > > Thanks for the feedback. > > Your change looks valid. We can use this and drop the max_t usage. > > Let me push this with your Suggested-by credits. You can take my Signed-off-by: Stephen Boyd <swboyd@chromium.org> and either squash it in or make a follow-up.
On 7/29/2024 1:08 PM, Stephen Boyd wrote: > Quoting Abhinav Kumar (2024-07-29 11:28:35) >> >> Thanks for the feedback. >> >> Your change looks valid. We can use this and drop the max_t usage. >> >> Let me push this with your Suggested-by credits. > > You can take my > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > and either squash it in or make a follow-up. Done, I have squashed it into this. I have also re-tested and since the logic is the same, I have retained Dmitry's Tested-by
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index a916b5f3b317..56ce5e4008f8 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -397,6 +397,7 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) { struct drm_display_mode *drm_mode; struct dp_panel_private *panel; + u32 max_supported_bpp; drm_mode = &dp_panel->dp_mode.drm_mode; @@ -423,8 +424,10 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel) drm_mode->clock); drm_dbg_dp(panel->drm_dev, "bpp = %d\n", dp_panel->dp_mode.bpp); - dp_panel->dp_mode.bpp = max_t(u32, 18, - min_t(u32, dp_panel->dp_mode.bpp, 30)); + max_supported_bpp = dp_panel_get_mode_bpp(dp_panel, dp_panel->dp_mode.bpp, + dp_panel->dp_mode.drm_mode.clock); + dp_panel->dp_mode.bpp = max_t(u32, 18, max_supported_bpp); + drm_dbg_dp(panel->drm_dev, "updated bpp = %d\n", dp_panel->dp_mode.bpp);
Currently the DP driver hard-codes the max supported bpp to 30. This is incorrect because the number of lanes and max data rate supported by the lanes need to be taken into account. Replace the hardcoded limit with the appropriate math which accounts for the accurate number of lanes and max data rate. Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_panel.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)