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

[Jose Abreu]

On 19-07-2017 20:21, John Stultz wrote:
> 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.

Ah, I meant drm_mode_copy(), sorry.

>
>
>>> +
>>> +     /*
>>> +      * 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?

Well, from the docs we get for the return value: "True if an
acceptable configuration is possible, false if the modeset
operation should be rejected." So, maybe return immediately
MODE_BAD ?

Best regards,
Jose Miguel Abreu

>
> thanks
> -john