[RFC PATCH 3/3] drm/msm: filter out modes for DP/eDP bridge having unsupported clock

[Abhinav Kumar]

On 9/7/2022 6:12 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2022-08-29 20:33:09)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index bfd0aeff3f0d..8b91d8adf921 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -117,6 +117,7 @@ struct dp_display_private {
>>
>>          bool wide_bus_en;
>>
>> +       int max_ext_pclk;
> 
> Same signed comment as before.

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;
> 
>>          struct dp_audio *audio;
>>   };
>>
>> @@ -986,8 +987,15 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
>>          if (dp->is_edp)
>>                  return MODE_OK;
>>
>> -       if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
>> -               return MODE_CLOCK_HIGH;
>> +       /*
>> +        * If DP/eDP supports HPD natively or through a bridge, need to make
>> +        * sure that we filter out the modes with pixel clock higher than the
>> +        * chipset capabilities
>> +        */
>> +       if ((bridge->ops & DRM_BRIDGE_OP_HPD) ||
>> +                       (dp->next_bridge && (dp->next_bridge->ops & DRM_BRIDGE_OP_HPD)))
> 
> Doesn't drm_bridge_chain_mode_valid() already run the mode_valid bridge
> function for all bridges in the chain? I don't understand why we need to
> peek at the bridge or the next bridge and only filter in that case. Why
> can't we always filter modes here? Do we want to allow higher pixel clks
> than the chip supports?

The next bridge does not know about the max capability of the previous 
bridge.

If the DP is used as a built-in display, we dont want this filter.

If the DP is used as the external display either directly or with the 
next/last bridge as the pluggable one, then we want to apply this filter.

The reason we cant always filter modes here is that lets say the DP is 
being used as a built-in display, then this filter is not needed.

Now coming to the HPD part of the next bridge, its not necessary that we 
use the DP bridge's HPD. We could be using the external bridge's HPD.

Like the DSI patch, I should change this to check the last bridge's HPD 
bridge op.

> 
>> +               if (mode->clock > dp_display->max_ext_pclk)
>> +                       return MODE_CLOCK_HIGH;
>>