[PATCH RFC] drm/atomic: Fall back to legacy call back in *_connector_dpms()

[Daniel Vetter]

On Mon, Feb 15, 2016 at 10:47 AM, Jyri Sarha <jsarha at ti.com> wrote:
> On 02/12/16 19:15, Ville Syrjälä wrote:
>>
>> On Fri, Feb 12, 2016 at 07:08:46PM +0200, Tomi Valkeinen wrote:
>>>
>>> On 12/02/16 17:34, Daniel Vetter wrote:
>>>>
>>>> On Fri, Feb 12, 2016 at 05:17:19PM +0200, Jyri Sarha wrote:
>>>>>
>>>>> Fall back to legacy drm_helper_connector_dpms() call back in
>>>>> drm_atomic_helper_connector_dpms() if DRIVER_ATOMIC feature is not
>>>>> present.
>>>>>
>>>>> Calling drm_atomic_helper_connector_dpms() from non atomic driver
>>>>> causes undefined behavior. This is a problem with componentized
>>>>> encoder/connector drivers that may be bound to both atomic and
>>>>> non atomic drivers.drm-next-tilcdc-fixes
>>>>>
>>>>> Signed-off-by: Jyri Sarha <jsarha at ti.com>
>>>>> ---
>>>>> This is just an alternative to this:
>>>>>
>>>>> https://lists.freedesktop.org/archives/dri-devel/2016-January/098867.html
>>>>
>>>>
>>>> I like the linked patch much better, since non-atomic really should die.
>>>> Inflicting non-atomic on atomic helpers is bad imo, so nack from me on
>>>> this patch here.
>>>
>>>
>>> I mostly agree, but we are in a transition period from non-atomic to
>>> atomic. When all drivers have been converted to atomic, it should be
>>> easy to grep for code using DRIVER_ATOMIC (or similar), and remove all
>>> the non-atomic support code.
>>>
>>> In my opinion it should be considered case by case if the
>>> non-atomic/atomic compat code should be added to the generic functions
>>> or to all the drivers using that functionality.
>>>
>>> In this case, if it only affects tda998x (but does it?), perhaps the
>>> patch in the link is better.
>>
>>
>> Maybe we need a hybrid helper that just calls the atomic
>> or non-atomic helper approppriately.
>>
>
> Would it actually be better if the legacy callback would automatically call
> atomic version if the driver has DRIVER_ATOMIC set?
>
> I do not have any strong opinions on this, just as long as one of the
> patches gets merged. With quick grepping I can see that there are several
> DRM component drivers, but I have no idea if anything else but the tda998x
> is being used by more than one DRM master driver.

I prefer open-coding since atomic vs. legacy is just one part of the
entire problem that the last element in the chain needs to register
the drm_connector, but in coordination with everyone else in the
chain. E.g. if you have upscale properties or anything else like that
it gets a lot more complicated, especially around the question of "who
owns drm_connector_state".

Given that I think it's better to add driver-private code for special
cases until we have a few examples and can go about creating a useful
helper library for this problem space. Instead of an inconsistent pile
of hacks enshrined forever.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch