[PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

Thank you for your inputs.

I liked all the suggestions, hence I have incorporated those and pushed 
a v2.

Thanks

Abhinav

On 3/13/2022 7:50 AM, Laurent Pinchart wrote:
> Hi Abhinav
> 
> On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
>> On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
>>> On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
>>>> Make changes to rcar_du driver to start using drm_encoder pointer
>>>> for drm_writeback_connector.
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal at intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> index c79d125..03930ad 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>>    {
>>>>    	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>>>>    
>>>> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>> +	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
>>>
>>> Where is this freed ?
>>
>> You are right, this isnt. Looking more into this, it seems like moving
>> the allocation of encoder to drm_writeback.c for cases which dont pass a
>> real encoder is much better so that I will not have to add alloc() /
>> free() code for all the vendor driver changes which is what I originally
>> thought in my RFC but changed my mind because of below.
> 
> Yes, I think that would be better indeed. You could even skip the
> dynamic allocation, you could have
> 
> 	struct drm_encoder *encoder;
> 	struct drm_encoder internal_encoder;
> 
> in drm_writeback_connector, and set
> 
> 	wb->encoder = &wb->internal_encoder;
> 
> when the user doesn't pass an encoder.
> 
>>>> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>
>> Do you think we can just move usage of wb_conn->encoder->possible_crtcs
>> just right after drm_writeback_connector_init() so that it wont crash?
> 
> How about adding a possible_crtcs argument to
> drm_writeback_connector_init() (to cover existing use cases), and adding
> a new drm_writeback_connector_init_with_encoder() that would get an
> encoder pointer (and expect possible_crtcs, as well as all the other
> appropriate encoder fields, having been initialized) ?
> 
>> 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>> 199 			   struct rcar_du_crtc *rcrtc)
>> 200 {
>> 201 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>> 202
>> 203 	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> 204 	drm_connector_helper_add(&wb_conn->base,
>> 205 				 &rcar_du_wb_conn_helper_funcs);
>> 206
>> 207 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>> 208 					    &rcar_du_wb_conn_funcs,
>> 209 					    &rcar_du_wb_enc_helper_funcs,
>> 210 					    writeback_formats,
>> 211 					    ARRAY_SIZE(writeback_formats));
>> 212 }
>>
>>>>    	drm_connector_helper_add(&wb_conn->base,
>>>>    				 &rcar_du_wb_conn_helper_funcs);
>>>>    
>