Re: [RFC][PATCH v3] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate

[John Stultz]

On Wed, Jul 19, 2017 at 3:16 AM, Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> wrote:
> Hi John,
>
>
> On 18-07-2017 18:59, John Stultz wrote:
>> Currently the hikey dsi logic cannot generate accurate byte
>> clocks values for all pixel clock values. Thus if a mode clock
>> is selected that cannot match the calculated byte clock, the
>> device will boot with a blank screen.
>>
>> This patch uses the new mode_valid callback (many thanks to
>> Jose Abreu for upstreaming it!) to ensure we don't select
>> modes we cannot generate.
>>
>> Also, since the ade crtc code will adjust the mode in mode_set,
>> this patch also adds a mode_fixup callback which we use to make
>> sure we are validating the mode clock that will eventually be
>> used.
>>
>> Many thanks to Jose and Daniel for recent feedback. I think this
>> version is looking much nicer. But I'd still welcome any feedback
>> or suggestions!
>>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Rob Clark <robdclark@xxxxxxxxx>
>> Cc: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
>> Cc: Xinliang Liu <z.liuxinliang@xxxxxxxxxxxxx>
>> Cc: Rongrong Zou <zourongrong@xxxxxxxxx>
>> Cc: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
>> Cc: Chen Feng <puck.chen@xxxxxxxxxxxxx>
>> Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx>
>> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> ---
>> v2: Reworked to calculate if modeclock matches the phy's
>>     byteclock, rather then using a whitelist of known modes.
>>
>> v3: Reworked to check across all possible crtcs (even though for
>>     us there is only one), and use mode_fixup instead of a custom
>>     function, as suggested by Jose and Daniel.
>> ---
>>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 62 +++++++++++++++++++++++++
>>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++
>>  2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> index f77dcfa..d7b5820 100644
>> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> @@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
>>       dsi->enable = true;
>>  }
>>
>> +static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
>> +                                     const struct drm_display_mode *mode)
>> +{
>> +     struct dw_dsi *dsi = encoder_to_dsi(encoder);
>> +     struct mipi_phy_params phy;
>> +     u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>> +     u32 req_kHz, act_kHz, lane_byte_clk_kHz;
>> +
>> +     /* Calculate the lane byte clk using the adjusted mode clk */
>> +     memset(&phy, 0, sizeof(phy));
>> +     req_kHz = mode->clock * bpp / dsi->lanes;
>> +     act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
>> +     lane_byte_clk_kHz = act_kHz / 8;
>> +
>> +     DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",
>> +                     mode->hdisplay, mode->vdisplay, bpp,
>> +                     drm_mode_vrefresh(mode), mode->clock);
>> +
>> +     /*
>> +      * Make sure the adjused mode clock and the lane byte clk
>> +      * have a common denominator base frequency
>> +      */
>> +     if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {
>> +             DRM_DEBUG_DRIVER("OK!\n");
>> +             return MODE_OK;
>> +     }
>> +
>> +     DRM_DEBUG_DRIVER("BAD!\n");
>> +     return MODE_BAD;
>> +}
>> +
>> +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
>> +                                     const struct drm_display_mode *mode)
>> +
>> +{
>> +     const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
>> +     struct drm_crtc *crtc = NULL;
>> +     struct drm_display_mode adj_mode;
>> +     int ret;
>
> This int should be an enum drm_mode_status ...
>
>> +
>> +     memcpy(&adj_mode, mode, sizeof(adj_mode));
>
> Maybe you should move this to the loop so that you pass a "clean"
> adjusted mode (i.e. adj_mode == mode) for each crtc. You could
> also use drm_mode_duplicate()/drm_mode_destroy() ...

Ah! Good catch! Thanks for that!

What is the benefit of drm_mode_duplicate over just using memcpy? I
see there's some base.id and head pointers that are kept unique, but
we're not touching those for this case. The extra allocating/freeing
seems a bit needless.


>> +
>> +     /*
>> +      * The crtc might adjust the mode, so go through the
>> +      * possible crtcs (technically just one) and call
>> +      * mode_fixup to figure out the adjusted mode before we
>> +      * validate it.
>> +      */
>> +     drm_for_each_crtc(crtc, encoder->dev) {
>> +             crtc_funcs = crtc->helper_private;
>> +             if (crtc_funcs && crtc_funcs->mode_fixup)
>> +                     ret = crtc_funcs->mode_fixup(crtc, mode,
>> +                                                     &adj_mode);
>
> No return check?

Hrm. Actually, not sure what to do if mode_fixup failed there. I guess
print a warning and validate the unadjusted mode? Other suggestions?

thanks
-john