Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

From: Jyri Sarha <jsarha@ti.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>, dri-devel@lists.freedesktop.org
Cc: laurent.pinchart@ideasonboard.com
Subject: Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support
Date: Tue, 10 May 2016 00:16:45 +0300	[thread overview]
Message-ID: <5730FE3D.3030407@ti.com> (raw)
In-Reply-To: <57309A38.6000702@ti.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 2116 bytes --]

On 05/09/16 17:10, Tomi Valkeinen wrote:
> Hi Jyri,
> 
> On 11/04/16 19:46, Jyri Sarha wrote:
>> The LCDC in its simplicity does not fit too well into DRM atomic
>> modeset abstractions. I wonder if I am doing the right thing in
>> implementing the dummy primary plane and in implementing
>> mode_set_nofb() crtc helper when the crtc actually needs the
>> framebuffer to be there when configuring it. See individual patch
>> descriptions for details. There is still lot of room for cleaning up
>> but I would first like to know if I am moving at all to the right
>> direction.
> 
> I'm no expert on atomic modesetting, but here are my comments/questions:
> 
> You add drm_crtc_vblank_off() to crtc_destroy() callback. Why is that? I
> think you should call drm_crtc_vblank_on() in crtc_enable(), and
> vblank_off in disable.
> 

I did not really think of that. I'll make the change.

> You remove the setting of tilcdc_crtc_helper_funcs.dpms, but leave the
> tilcdc_crtc_dpms, which you use elsewhere. I think all dpms stuff should
> be removed from crtc, as it's all either "enable" or "disable". Git grep
> shows that in omapdrm, there's just one reference to dpms, in
> omap_connector.c: .dpms = drm_atomic_helper_connector_dpms
> 

I left that for subsequent clean up after general approach is accepted.

> It's not clear to me if a (primary) plane is required with atomic
> universal planes and modesetting or not... I guess it is, when thinking
> of how e.g. a framebuffer is configured to be shown on a screen when
> using the atomic modesetting uapi.
> 

As modeset_nofb is the preferred call back for setting the mode, and it
does not require any fb or plane, I needed something to express the
mandatory framebuffer. The primatry plane just wast the first solution
that came to my mind and it appeared to work.

> But if it is required, it makes me wonder, are there other HWs out there
> without any planes? The dummy plane implementation you added is not
> complex, but is it something that should be implemented with DRM helper
> funcs?
> 
>  Tomi
> 

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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