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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
