<pre>
Hi Eugen,

On Mon, 2023-07-31 at 11:32 +0300, Eugen Hristev wrote:
> On 7/31/23 11:21, Jason-JH Lin (林睿祥) wrote:
> > Hi Eugen,
> >
> > Thanks for the reviews.
> >

snip...

> > > > +if (temp_drm_priv->data->main_len)
> > > > +all_drm_priv[0] =
> > > > temp_drm_priv;
> > > > +else if (temp_drm_priv->data->ext_len)
> > > > +all_drm_priv[1] =
> > > > temp_drm_priv;
> > > > +else if (temp_drm_priv->data-
> > > > >third_len)
> > > > +all_drm_priv[2] =
> > > > temp_drm_priv;
> > > > +}
> > >
> > > Previously the code was assigning stuff into all_drm_priv[cnt]
> > > and
> > > incrementing it.
> > > With your change, it assigns to all_drm_priv[0], [1], [2]. Is
> > > this
> > > what
> > > you intended ?
> >
> > Because dev_get_drvdata(drm_dev) will get the driver data by
> > drm_dev.
> > Each drm_dev represents a display path.
> > e,g.
> > drm_dev of "mediatek,mt8195-vdosys0" represents main path.
> > drm_dev of "mediatek,mt8195-vdosys1" represents ext path.
> >
> > So we want to make sure all_drm_priv[] store the private data in
> > the order of display path, such as:
> > all_drm_priv[0] = the private data of main display
> > all_drm_priv[1] = the private data of ext display
> > all_drm_priv[2] = the private data of third display
>
> If you have such a hard requirement for keeping elements in an
> array,
> you are better having
> drm_priv_main_display
> drm_priv_ext_display
> drm_priv_third_display
>
> Keeping them indexed in a three elements array by having no logical
> connection between the number [0,1,2] and the actual displays that
> you
> want to save is a bit confusing.
>

Yes, I think it was a bit confusing.

But we don't know which drm_priv will go into this function first and
we want to store all drm_priv into the same array.
So it has come to this.

> One other option which I don't know if it's better or not is to have
> macros to hide your indexed approach:
> all_drm_priv[MAIN_DISPLAY] ...
> etc.
>

Thanks for your advice.
I'll try to use macros to make it better and more readable.

> >
> >
> > > If this loop has second run, you will reassign to all_drm_priv
> > > again
> > > ?
> >
> > Because the previous code will store all_drm_priv[] in the order of
> > mtk_drm_bind() was called.
> >
> > If drm_dev of ext path bound earlier than drm_dev of main path,
> > all_drm_priv[] in mtk_drm_get_all_drm_priv() may be re-assigned
> > like
> > this:
> > all_drm_priv[0]->all_drm_priv[0] = private data of ext path
> > all_drm_priv[1]->all_drm_priv[0] = private data of ext path
> > all_drm_priv[0]->all_drm_priv[1] = private data of main path
> > all_drm_priv[1]->all_drm_priv[1] = private data of main path
> >
> > But we expect all_drm_priv[] be re-assigned like this:
> > all_drm_priv[0]->all_drm_priv[0] = private data of main path
> > all_drm_priv[1]->all_drm_priv[0] = private data of main path
> > all_drm_priv[0]->all_drm_priv[1] = private data of ext path
> > all_drm_priv[1]->all_drm_priv[1] = private data of ext path
>
> This expectation does not appear to be really enforced in your code.
> You have a driver that keeps an array with all_drm_priv[], in which
> you can have main path or ext path. Then it's natural that they
> might
> have whichever order in the array you are placing them into.
> If you have a hard enforced order of keeping elements in your array,
> then an indexed array is not the best option here.
> You can either: move to a different type of array , with macros for
> indexes into the array, or, store a second array/field which keeps
> the
> index in which you saved each element.
>
> This is just my opinion , by looking at your code.
>

There is another statement in mtk_drm_kms_init() like this:

for (i = 0; i < MAX_CRTC; i++) {
for (j = 0; j< private->data->mmsys_dev_num; j++) {
priv_n = private->all_drm_private[j];

if (i == 0 && priv_n->data->main_len) {
...
} else if (i == 1 && priv_n->data->ext_len) {
...
} else if (i == 2 && priv_n->data->third_len) {
...
}
}
}

So we need to make sure that each element in all_drm_priv[] has only
one path data:
all_drm_priv[0] has main_path data only
all_drm_priv[1] has ext_path data only
all_drm_priv[2] has third_path data only

I think it would take quite a bit of effort to change this array usage.

> > > I would expect you to take `cnt` into account.
> > > Also, is it expected that all_drm_priv has holes in the array ?
> >
> > Each drm_dev will only called mtk_drm_bind() once, so all holes
> > will be filled after all drm_dev has called mtk_drm_bind().
> >
> > Do you agree with this statement? :)
>
> At the moment I cannot agree nor disagree, I don't know the code
> well
> enough. But what I can say, is that you should not rely on future
> calls
> of the function to fill up your array correctly.
>

I agree with your opinion, but at the moment, I just want to fix the
issue first by having a less modification.

I'll try to use macros to replace the array index and I'll add more
description into commit message to express the current limitation in
mtk_drm_kms_init().

Thanks again~

Regards,
Jason-JH.Lin

> >
> > Regards,
> > Jason-JH.Lin
> >
> > >
> > > Eugen
> > >
> > >
> > >
> > > > }
> > > >
> > > > if (drm_priv->data->mmsys_dev_num == cnt) {
> > >
> > >
>
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->