Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

[Yakir Yang <ykk@rock-chips.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3653 bytes --]

Hi Thierry

When I'm preparing v3 series, I meet some trobules from your comment, 
wish you could give some advise?  ;)

在 2015/8/10 18:00, Thierry Reding 写道:
> On Sat, Aug 08, 2015 at 11:54:38AM +0800, Yakir Yang wrote:
> [...]
>>          edp: edp@ff970000 {
> [...]
>>                  samsung,color-space = <0>;
>>                  samsung,dynamic-range = <0>;
>>                  samsung,ycbcr-coeff = <0>;
> I think these should also come from EDID, though I'm not sure if we
> store this in internal data structures yet.
>
>>                  samsung,color-depth = <1>;
> This is probably drm_display_info.bpc.
"samsung,color_space" and "samsung,color-depth"

The drm_display_info's color_formats and bpc indicate the monitor 
display ability, but
the edp driver could not take it as input video format directly.

For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
support in drm_display_info.color_formats and 16bit bpc support, but 
RK3288 crtc
driver could only output RGB & ITU formats, so finally 
analogix_dp-rockchip driver
config crtc to RGBaaa 10bpc mode.

In this sutiation, the analogix_dp core driver would pazzled by the 
drm_display_info,
can't chose the right color_space and bpc.

And this is the place that confused me, wish you could give some ideas 
about this one :-)
-------------------------------------------------------------------------------------------------

Besides, The dynamic_range and ycbcr_coeff haven't been record in 
drm_display_info, but
I though we can parse it by the video code.

The dynamic_range would have two values "CEA range" and "VESA range", so 
I though if
the currect mode have a no-zero vic (drm_match_cea_mode()) then config 
it to "CEA range",
otherwhise config it to "VESA range".

YCbCr Coefficients would have two values "ITU709" and "ITU601". I see 
dw_hdmi driver have
been set the colorimetry to ITU_601 when vic is 6/7/21/22/2/3/17/18, I 
thouht we can stole this
to analogix_dp driver.

    /* dynamic_range & colorimetry */
         vic = drm_match_cea_mode(mode);
         if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) ||
             (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) {
                 video_info->dynamic_range = CEA;
                 video_info->ycbcr_coeff = COLOR_YCBCR601;
         } else if (vic) {
                 video_info->dynamic_range = CEA;
                 video_info->ycbcr_coeff = COLOR_YCBCR709;
         } else {
                 video_info->dynamic_range = VESA;
                 video_info->ycbcr_coeff = COLOR_YCBCR709;
         }

I'm not sure whether this is right, also wish you could give some 
suggests ;)


>>                  samsung,link-rate = <0x0a>;
>>                  samsung,lane-count = <1>;
> And these should really be derived from values in the DPCD and adjusted
> (if necessary) during link training.
>
> Why would you ever want to hard-code the above?
>

And I though we should keep those DT properties now. I try to remove 
those DT
property, so analogix dp driver would always use the max link_rate and 
lane_count
which read from dpcd. For my 2K DP TV, link rate would reach to 5.4Gbps, 
lane cout
would reach 4 lances, if so analogix dp driver could not light my DP TV 
up any more.

After that I found that RK3288 eDP TRM have indicated some limites about 
those
that RK3288 only support 4 physical lanes of 2.7/1.62 Gbps/lane, which 
means 5.4Gbps
link rate is too high for RK3288.

So I think we could treate them as hardware max values, is it okay?

Thanks,
- Yakir

[-- Attachment #1.2: Type: text/html, Size: 5431 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel