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