[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <bacae46ecdacf71c47eaa8031ffdbff55e12f66c.camel@mediatek.com>
Date: Wed, 2 Aug 2023 07:05:55 +0000
From: Jason-JH Lin (林睿祥)
<Jason-JH.Lin@...iatek.com>
To: "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>,
"eugen.hristev@...labora.com" <eugen.hristev@...labora.com>
CC: "linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Singo Chang (張興國)
<Singo.Chang@...iatek.com>,
Johnson Wang (王聖鑫)
<Johnson.Wang@...iatek.com>,
Jason-ch Chen (陳建豪)
<Jason-ch.Chen@...iatek.com>,
Shawn Sung (宋孝謙)
<Shawn.Sung@...iatek.com>,
Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH v7 2/4] drm/mediatek: Fix using wrong drm private data to
bind mediatek-drm
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) {
> > >
> > >
>
>
Powered by blists - more mailing lists