On 21/04/2019 00:39, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
>> On 15/04/2019 11:36, Andrzej Hajda wrote:
>>
>>>> +static int tc_main_link_disable(struct tc_data *tc)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + dev_dbg(tc->dev, "link disable\n");
>>>> +
>>>> + tc_write(DP0_SRCCTRL, 0);
>>>> + tc_write(DP0CTL, 0);
>>>
>>> The same register is set in stream_disable, is it correct? Looks
>>> suspicious, disabling stream and link should be different things.
>>
>> Good point. It's probably not correct. With these patches, the link and
>> stream are always enabled and disabled together, so it shouldn't cause
>> any problems at the moment.
>>
>> During my debugging and development, at some point I had a version where
>> I enabled the link right away when we got HPD high, mostly to help my
>> debugging. That's why I separated link and stream setup (although I
>> think that's a logical design in any case).
>>
>> However, I never did try disabling only stream, and keeping the link up,
>> so it may well be non-functional. Or, well, it clearly is
>> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
>>
>> So this is mostly about just adding the architecture to have separate
>> link/stream handling, but the functionality is not there. I should
>> perhaps add a comment to the code to point this out.
>
> This makes me a bit uneasy about the rework, as the
> tc_main_link_enable() function doesn't enable the link (it doesn't set
> the DP_EN bit in DP0CTL), and stream disabling separately from link
> disabling is broken. Is this fixable ?
tc_main_link_enable() does set DP_EN.
It's true that managing the link and stream separately isn't working,
but it wasn't there earlier either, and it still isn't, as the driver
never enables/disables the link/stream separately. We need more code and
logic to manage them separately, and this series just starts the work by
trying to organize the code better.
I'll look at tc_stream_disable(), as tc_write(DP0CTL, 0) there does not
look correct. Probably we can just clear VID_EN.
Afaics, this series should not introduce any issues, but fixes many
issues that were present. There's lots more to do, and there's another
series from Andrey that cleans up many things that I didn't touch in
this series. And after Andrey's cleanups, I think we can then start
looking at adding more logic like the separate link/stream enabling if
needed.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
