Re: [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: "Kandpal, Suraj" <suraj.kandpal@intel.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Carsten Haitzler <carsten.haitzler@arm.com>,
	"Nikula, Jani" <jani.nikula@intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	David Airlie <airlied@linux.ie>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	"Murthy, Arun R" <arun.r.murthy@intel.com>
Subject: Re: [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Date: Tue, 8 Mar 2022 11:44:07 -0800	[thread overview]
Message-ID: <4daece94-c438-29dd-ef9d-e4c4c06187c4@quicinc.com> (raw)
In-Reply-To: <MWHPR11MB1741B36C4BA59CDF84D9F52EE3099@MWHPR11MB1741.namprd11.prod.outlook.com>

Hi Suraj

On 3/8/2022 6:30 AM, Kandpal, Suraj wrote:
> Hi Abhinav,
>>> Hi,
>>>>> Hi,
>>>>>>> Hi Abhinav,
>>>>>>>> Hi Laurent
>>>>>>>>
>>>>>>>> Ok sure, I can take this up but I need to understand the proposal
>>>>>>>> a little bit more before proceeding on this. So we will discuss
>>>>>>>> this in another email where we specifically talk about the
>>>> connector changes.
>>>>>>>>
>>>>>>>> Also, I am willing to take this up once the encoder part is done
>>>>>>>> and merged so that atleast we keep moving on that as MSM WB
>>>>>>>> implementation can proceed with that first.
>>>>>>>>
>>>>>>>> Hi Jani and Suraj
>>>>>>>>
>>>>>>>> Any concerns with splitting up the series into encoder and
>>>>>>>> connector OR re- arranging them?
>>>>>>>>
>>>>>>>> Let me know if you can do this OR I can also split this up
>>>>>>>> keeping Suraj's name in the Co-developed by tag.
>>>>>>> I was actually thinking of floating a new patch series with both
>>>>>>> encoder and connector pointer changes but with a change in
>>>>>>> initialization functions wherein we allocate a connector and
>>>>>>> encoder incase the driver doesn’t have its own this should
>>>>>>> minimize the effect on other drivers already using current drm
>>>>>>> writeback framework and also
>>>>>> allow i915 to create its own connector.
>>>>>>
>>
>> I was proposing to split up the encoder and connector because the
>> comments from Laurent meant we were in agreement about the encoder
>> but not about the connector.
>>
>> I am afraid another attempt with the similar idea is only going to stall the
>> series again and hence i gave this option.
> 
> Seems like this patch series currently won't be able to move forward with the
> connector pointer change so I guess you can split the series to encoder pointer
> change where we optionally create encoder if required ,keeping my name in
> co-developed tag so that msm can move forward with its implementation and
> then we can work to see how the connector issue can be bypassed.
> 
> Best Regards,
> Suraj Kandpal

Thanks, let me re-spin this and will keep your name as co-developer.
Should be able to get it on the list in a week.

Thanks

Abhinav
>> Eventually its upto Laurent and the other maintainers to guide us forward on
>> this as this series has been stalled for weeks now.
>>
>>>>>> I thought that Laurent was quite strict about the connector. So I'd
>>>>>> suggest targeting drm_writeback_connector with an optionally
>>>>>> created encoder and the builtin connector
>>>>> In that case even if we target that i915 will not be able to move
>>>>> forward with its implementation of writeback as builtin connector
>>>>> does not work for us right now as explained earlier, optionally
>>>>> creating encoder and connector will help everyone move forward and
>>>>> once done
>>>> we
>>>>> can actually sit and work on how to side step this issue using
>>>>> Laurent's suggestion
>>>>
>>>> This is up to Laurent & other DRM maintainers to decide whether this
>>>> approach would be accepted or not.
>>>> So far unfortunately I have mostly seen the pushback and a strong
>>>> suggestion to stop treating each drm_connector as i915_connector.
>>>> I haven't checked the exact details, but IMO adding a branch if the
>>>> connector's type is DRM_CONNECTOR_VIRTUAL should be doable.
>>> Bringing in the change where we don’t treat each drm_connector as an
>>> intel_connector or even adding a branch which checks if drm_connector
>>> is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector
>>> is quite a lot of work for i915.
>>> That's why I was suggesting if for now we could move forward with
>>> optionally creating both encoder and connector minimizing affects on
>>> other drivers as well as allowing us to move forward.
>>> What do you think, Launrent?
>>>
>>>>
>>>>>>
>>>>>>> We can work on Laurent's suggestion but that would first require
>>>>>>> the initial RFC patches and then a rework from both of our drivers
>>>>>>> side to see if they gel well with our current code which will take
>>>>>>> a considerable amount of time. We can for now however float the
>>>>>>> patch series up which minimally affects the current drivers and
>>>>>>> also allows
>>>>>>> i915 and msm to move forward with its writeback implementation off
>>>>>>> course with a proper documentation stating new drivers shouldn't
>>>>>>> try to
>>>>>> make their own connectors and encoders and that drm_writeback will
>>>>>> do that for them.
>>>>>>> Once that's done and merged we can work with Laurent on his
>>>>>>> proposal to improve the drm writeback framework so that this issue
>>>>>>> can be side
>>>>>> stepped entirely in future.
>>>>>>> For now I would like to keep the encoder and connector pointer
>>>>>>> changes
>>>>>> together.
>>>>>
>>>>