<pre>
Hi Eugen,

Thanks for the reviews.

On Fri, 2023-07-28 at 11:47 +0300, Eugen Hristev wrote:
> Hi,
>
> On 7/27/23 19:41, Jason-JH.Lin wrote:
> > Add checking the length of each data path before assigning drm
> > private
> > data into all_drm_priv array.
> >
> > Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195
> > multi mmsys support")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 16 +++++++++++++---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 249c9fd6347e..d2fb1fb4e682 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -351,6 +351,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> > {
> > struct mtk_drm_private *drm_priv = dev_get_drvdata(dev);
> > struct mtk_drm_private *all_drm_priv[MAX_CRTC];
> > +struct mtk_drm_private *temp_drm_priv;
> > struct device_node *phandle = dev->parent->of_node;
> > const struct of_device_id *of_id;
> > struct device_node *node;
> > @@ -373,9 +374,18 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> > if (!drm_dev || !dev_get_drvdata(drm_dev))
> > continue;
> >
> > -all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
> > -if (all_drm_priv[cnt] && all_drm_priv[cnt]-
> > >mtk_drm_bound)
> > -cnt++;
> > +temp_drm_priv = dev_get_drvdata(drm_dev);
> > +if (temp_drm_priv) {
> > +if (temp_drm_priv->mtk_drm_bound)
> > +cnt++;
> > +
> > +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 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

> 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? :)

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><!--}-->