Re: [PATCH v7 2/4] drm/mediatek: Fix using wrong drm private data to bind mediatek-drm

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

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) {
> > > 
> > > 
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel