On Wed, May 9, 2018 at 1:52 PM, Satendra Singh Thakur
<satendra.t@xxxxxxxxxxx> wrote:
> On Thu, May 08, 2018 at 16:28:30 +0530, Satendra Singh Thakur wrote:
>> On Thu, May 07, 2018 at 15:46:02 +0200, Daniel Vetter wrote:
>> > On Thu, May 03, 2018 at 01:53:55PM +0530, Satendra Singh Thakur wrote:
>> > > 1.There is a function in drm-core to calculate display timing parameters:
>> > > horizontal front porch, back porch, sync length,
>> > > vertical front porch, back porch, sync length and
>> > > clock in Hz.
>> > > However, some drivers are still calculating these parameters themselves.
>> > > Therefore, there is a duplication of the code.
>> > > This patch series replaces this redundant code with the function
>> > > drm_display_mode_to_videomode.
>> > > This removes nearly 100 redundant lines from the related drivers.
>> > > 2.For some drivers (sun4i) the reverse helper
>> > > drm_display_mode_from_videomode is used.
>> > > 3.For some drivers it replaces arithmatic operators (*, /) with shifting
>> > > operators (>>, <<).
>> > > 4.For some drivers DRM_MODE_FLAG_* are replaced with DISPLAY_FLAGS_* flags.
>> > > 5.These changes apply to following crtc and encoder drivers:
>> > > atmel-hlcdc
>> > > bridge-tc358767
>> > > exynos-dsi
>> > > fsl-dcu
>> > > gma500-mdfld_dsi_dpi
>> > > hisilicon-kirin_dsi, ade
>> > > meson-encoder
>> > > pl111-display
>> > > sun4i-tv
>> > > ti lcdc
>> > > tegra dc
>> > > mediatek dpi dsi
>> > > bridge-adv7533
>> >
>> > The drm_mode_to_videomode helper is meant for interop between drm and v4l,
>> > which have different internal structures to represent modes.
>> >
>> > For drivers that only use drm I think the better option would be to add
>> > these fields to struct drm_display_mode as another set of crtc_* values
>> > (the computed values are stored in crtc_ prefixed members). And compute
>> > front/back porch in drm_mode_set_crtcinfo.
>> >
>> > Then we can use these new drm_display_mode->crtc_h|vfront|back_porch
>> > fields in all the drivers you're changing. This way you avoid having to
>> > change all the drm drivers to use v4l #defines.
>> >
>> > Thanks,
>> > Daniel
>>
>> Hi Daniel,
>> Thanks for the comments.
>> I will look into it.
>>
>> Thanks
>> -Satendra
>
> Hi Daniel,
> Thanks for the comments.
> Please find below my understanding in this direction.
>
> 1. Currently struct videomode is being used in 50 drm drivers and 14 fbdev drivers.
> Since, it's already being used by so many drm drivers, that is the reason
> these fbdev objects (struct videmode and func drm_display_mode_to_videomode) were used in this patch series.
The biggest contributor for that seems to be of_get_videomode. We
should probably have a of_drm_get_display_mode helper, which
automatically converts the of/dt video description into the drm
display mode structure.
And I found way less than 50 drm drivers using videomode, much less if
we ignore of.
> 2. Anyway, if fbdev related objects (struct/func) are not encouraged in drm drivers, then we may add
> h/v front/back porches in struct drm_display_mode as adviced by you.
>
> 3. We can calculate these params in func drm_mode_set_crtcinfo at the end of it.
> int drm_mode_set_crtcinfo ()
> {
> .
> .
> crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay;
> crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay;
> .
> .
> crtc_clock_hz = crtc_clock*1000;
> };
>
> 4. Normally mode is programmed in HW in following callbacks of crtc and encoder drivers
> -mode_set
> -mode_set_nofb
> -atomic_enable
>
>
> Normally drm_mode_set_crtcinfo is used in
> -mode_fixup callbacks (CBs)
> of encoder and crtc drivers.
>
> if mode_fixup CBs are called before mode_set CBs then
> drm_mode_set_crtcinfo is right place to calculate sync/porch params.
> We can use crtc_hfront/back_porches directly and program them to HW
> in above mentioned callbacks.
>
> int my_mode_set ()
> {
> REG_WRITE(crtc_hfront_porch);
> REG_WRITE(crtc_hback_porch);
> .
> .
> }
Agreed with your plan up to point 5 here.
> 6. However, if these params are being modified after calling drm_set_crtcinfo as mentioned below:
>
> bool amdgpu_atombios_encoder_mode_fixup(struct drm_encoder *encoder,
> const struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode)
> {
> struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
>
> /* set the active encoder to connector routing */
> amdgpu_encoder_set_active_device(encoder);
> ***drm_mode_set_crtcinfo(adjusted_mode, 0);****
>
> /* hw bug */
> if ((mode->flags & DRM_MODE_FLAG_INTERLACE)
> && (mode->crtc_vsync_start < (mode->crtc_vdisplay + 2)))
> adjusted_mode->crtc_vsync_start = adjusted_mode->crtc_vdisplay + 2;
>
> Then we may need to define new func like
>
> void drm_calc_hv_porches_sync()
> {
> crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay;
> crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay;
> .
> .
> crtc_clock_hz = crtc_clock*1000;
> } and call this func in mode_set*, atomic_enable CBS before programming the HW with this mode.
>
>
> Should I create patches around this idea ?
> Please let me know your comments.
I'm not sure about point 6. I think we should wait with coming up with
a solution to this problem once there's a clear need for it. Most
likely I think drivers who both need to adjust computed timings and
who need v/hfront/back porch just need to adjust everything on their
own. And we won't provide any additional helpers.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
