Date:   Tue, 14 Feb 2023 16:16:41 +0200
From:   Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To:     Alexander Stein <alexander.stein@ew.tq-group.com>
Cc:     dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Liu Ying <victor.liu@nxp.com>, marex@denx.de,
        shawnguo@kernel.org, s.hauer@pengutronix.de,
        krzysztof.kozlowski@linaro.org, robh+dt@kernel.org,
        linux-imx@nxp.com, krzysztof.kozlowski+dt@linaro.org,
        kernel@pengutronix.de, LW@karo-electronics.de
Subject: Re: [PATCH v3 3/6] drm: lcdif: Determine bus format and flags in
 ->atomic_check()
Message-ID: <Y+uXyfZLyZ7Bm4j8@intel.com>
References: <20230213085612.1026538-1-victor.liu@nxp.com>
 <20230213085612.1026538-4-victor.liu@nxp.com>
 <1755644.VLH7GnMWUR@steina-w>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1755644.VLH7GnMWUR@steina-w>
Precedence: bulk

On Tue, Feb 14, 2023 at 03:12:55PM +0100, Alexander Stein wrote:
> Hi Liu,
> 
> thanks for the update.
> 
> Am Montag, 13. Februar 2023, 09:56:09 CET schrieb Liu Ying:
> > Instead of determining LCDIF output bus format and bus flags in
> > ->atomic_enable(), do that in ->atomic_check().  This is a
> > preparation for the upcoming patch to check consistent bus format
> > and bus flags across all first downstream bridges in ->atomic_check().
> > New lcdif_crtc_state structure is introduced to cache bus format
> > and bus flags states in ->atomic_check() so that they can be read
> > in ->atomic_enable().
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > v2->v3:
> > * No change.
> > 
> > v1->v2:
> > * Split from patch 2/2 in v1. (Marek, Alexander)
> > * Add comment on the 'base' member of lcdif_crtc_state structure to
> >   note it should always be the first member. (Lothar)
> > 
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 138 ++++++++++++++++++++++--------
> >  1 file changed, 100 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c index e54200a9fcb9..294cecdf5439 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -30,6 +30,18 @@
> >  #include "lcdif_drv.h"
> >  #include "lcdif_regs.h"
> > 
> > +struct lcdif_crtc_state {
> > +	struct drm_crtc_state	base;	/* always be the first 
> member */
> 
> Is it strictly necessary that base is the first member? to_lcdif_crtc_state() 
> should be able to handle any position within the struct. I mean it's sensible 
> to put it in first place. But the comment somehow bugs me.

NULL pointers is where these things go bad if it't not at
offset 0. Unless you're willing to always handle those
explicitly.

-- 
Ville Syrj?l?
Intel