[RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu Sep 8 18:53:14 UTC 2022
Hi Stephen
On 9/7/2022 6:06 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2022-08-29 20:33:08)
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 39bbabb5daf6..3a06a157d1b1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -265,6 +265,11 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>> return ret;
>> }
>>
>> +void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk)
>
> Do we really need a 'setter' API for something like this? Why can't we
> directly access the constant value for the max clk in the function that
> uses it to limit modes?
That constant value is part of the DPU catalog. the value needs to be
passed from the DPU to DSI/DP for it to use. Hence the setter API.
In this RFC atleast, this is being modeled as a DPU catalog entry.
>
>> +{
>> + msm_dsi_host_set_max_extpclk(msm_dsi->host, max_ext_pclk);
>> +}
>> +
>> void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
>> {
>> msm_dsi_host_snapshot(disp_state, msm_dsi->host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 2a96b4fe7839..1be4ebb0f9c8 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -93,8 +93,9 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>> 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);
>> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
>> + const struct drm_display_mode *mode,
>> + struct drm_bridge *ext_bridge);
>> 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);
>> @@ -109,6 +110,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>> void msm_dsi_host_destroy(struct mipi_dsi_host *host);
>> int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>> struct drm_device *dev);
>> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk);
>> int msm_dsi_host_init(struct msm_dsi *msm_dsi);
>> int msm_dsi_runtime_suspend(struct device *dev);
>> int msm_dsi_runtime_resume(struct device *dev);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 57a4c0fa614b..4428a6a66ee1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -172,6 +172,9 @@ struct msm_dsi_host {
>> int dlane_swap;
>> int num_data_lanes;
>>
>> + /* max pixel clock when used with a bridge chip */
>> + int max_ext_pclk;
>
> Will pixel clock be negative? What units is this in? Hz?
This is in Khz. It cannot be negative. I was also thinking of an
unsigned int for this but the drm_display_mode's clock is an int so i
also kept it like this.
227 struct drm_display_mode {
228 /**
229 * @clock:
230 *
231 * Pixel clock in kHz.
232 */
233 int clock; /* in kHz */
234 u16 hdisplay;
>
>> +
>> /* from phy DT */
>> bool cphy_mode;
>>
>> @@ -2076,6 +2079,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>> return 0;
>> }
>>
>> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk)
>> +{
>> + struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +
>> + msm_host->max_ext_pclk = max_ext_pclk;
>> +}
>> +
>> int msm_dsi_host_register(struct mipi_dsi_host *host)
>> {
>> struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> @@ -2548,17 +2558,14 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>> return 0;
>> }
>>
>> -enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> - const struct drm_display_mode *mode)
>> +static enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> + const struct drm_display_mode *mode)
>> {
>> struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> struct drm_dsc_config *dsc = msm_host->dsc;
>> int pic_width = mode->hdisplay;
>> int pic_height = mode->vdisplay;
>>
>> - if (!msm_host->dsc)
>> - return MODE_OK;
>> -
>> 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);
>> @@ -2574,6 +2581,29 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> return MODE_OK;
>> }
>>
>> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
>> + const struct drm_display_mode *mode,
>> + struct drm_bridge *ext_bridge)
>> +{
>> + struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +
>> + /* TODO: external bridge chip with DSI having DSC */
>> + if (msm_host->dsc)
>> + return msm_dsi_host_check_dsc(host, mode);
>> +
>> + /* TODO: add same logic for non-dpu targets */
>> + if (!msm_host->max_ext_pclk)
>> + return MODE_OK;
>> +
>> + if (ext_bridge) {
>> + if (ext_bridge->ops & DRM_BRIDGE_OP_HPD)
>
> Nitpick: Collapse conditions
>
> if (ext_bridge && (ext_bridge->ops & DRM_BRIDGE_OP_HPD))
Ack
>
> Also, what does HPD have to do with this?
The documents referenced in the cover letter define the limits for
built-in and external displays.
This series is targetted only for external displays with an underlying
assumption that built-in displays are chosen at design time of the
product and the product spec should be kept in mind while choosing them.
But for external ( pluggable ) displays, this is not true as the
consumer can plug-in any monitor.
Now, there is no rule that DSI cannot be used as the external display
with a DSI to HDMI or DSI to DP bridge chip.
In those cases, we need to check if the ext_bridge has HPD support and
if so use this filtering of modes.
After discussing with Dmitry, I do agree though that instead of checking
the next bridge, I should be checking the last bridge in the chain instead.
So when i do push the next version, I should change this to check if the
last bridge has HPD support.
>
>> + if (mode->clock > msm_host->max_ext_pclk)
>> + return MODE_CLOCK_HIGH;
>> + }
>> +
>> + return MODE_OK;
>> +}
>> +
>> unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host)
>> {
>> return to_msm_dsi_host(host)->mode_flags;
More information about the dri-devel
mailing list